Fix deadlock when closing pipe

[mat007]

Hi!

This PR fixes a deadlock that we face every once in a while.

Thanks!

[thaJeztah]

@mat007 I think this PR fixes #257, correct? If so, can you add a fixes xxx to the PR description (not the commit message, as that tends to get noisy 😂)

[mat007]

I think this PR fixes #257, correct?

I don’t know if it fixes that one, I was investigating something else. I’m really not sure it’s related. 🤔

[thaJeztah]

Oh, sorry; someone pointed me to that ticket as the reason for maintaining a fork, so I thought this PR was related to that

[kevpar]

@mat007 can you explain what the deadlock is that you're seeing?

[mat007]

@mat007 can you explain what the deadlock is that you're seeing?

Sure!

The added test without the fix never terminates. Breaking under a debugger we see that the goroutine that called Close is blocked on

go-winio/pipe.go

Line 578 in bdc6c11

<-l.doneCh

while the ListenPipe one is waiting on

go-winio/pipe.go

Lines 462 to 463 in bdc6c11

	select {
	case <-l.closeCh:

The problem is that we have 2 readers for l.closeCh, with the other one being

go-winio/pipe.go

Line 447 in bdc6c11

case <-l.closeCh:

but we’re signalling it by writing a value to the channel, which can only be read by one reader.
This PR fixes this by closing the channel instead of writing to it.

[mat007]

@kevpar what do you think of my explanation? Do you need more info?
Thanks!

[kevpar]

@kevpar what do you think of my explanation? Do you need more info? Thanks!

Sorry, I just got back from holiday. I will take a look today and see if we can get this in!

[kevpar]

I've been doing some testing/thinking on this. Overall I think it looks good, just have a few pieces of feedback.

• Let's change closeCh and doneCh to type chan struct{}, since we don't actually care about the value being sent.
• In the <-l.closeCh case of (*win32PipeListener).Close, I think it's best to still do a <-l.doneCh before returning. This will ensure that if there are multiple concurrent calls to Close, the later calls will not return until doneCh is closed.
• In TestCloseRace, can we increase the number of iterations, perhaps to say, 1000? In my testing I sometimes found that it took until iteration 150+ before deadlocking, and we don't want to accidentally get a false negative from this test case if a deadlock is reintroduced.
• Also in TestCloseRace, I would suggset adding a log.Print statement that prints the iteration number that is completed at the end of the loop. This could help tell when we are deadlocking from test logs if we hit a problem in the future.

Thanks!

[kevpar]

Also, looks like there are some CI failures.

[thaJeztah]

Had a quick peek; linting errors look unrelated to the PR;

Warning: redefines-builtin-id: redefinition of the built-in function max (revive)
Warning: redefines-builtin-id: redefinition of the built-in function max (revive)
Error: printf: non-constant format string in call to (testing.TB).Fatalf (govet)
Error: directive `//nolint:errorlint` is unused for linter "errorlint" (nolintlint)

The go-generate one is more interesting, as there's a panic;

go run github.com/Microsoft/go-winio/tools/mkwinsyscall -output zsyscall_windows.go socket.go
  go run github.com/Microsoft/go-winio/tools/mkwinsyscall -output zsyscall_windows.go ./bind_filter.go
  go run golang.org/x/tools/cmd/stringer -type=Level -trimprefix=Level
  go: downloading golang.org/x/tools v0.11.0
  go: downloading golang.org/x/mod v0.12.0
  panic: runtime error: invalid memory address or nil pointer dereference [recovered]
  	panic: runtime error: invalid memory address or nil pointer dereference
  [signal 0xc0000005 code=0x0 addr=0x0 pc=0xbff0af]

I see 070c828 changed CI to not used fixed versions of Go, and oldstable moved to go1.22; I wonder if the panic is related to that as well;

Run actions/setup-go@v5
  with:
    go-version: oldstable
    cache: false
    check-latest: false
    token: ***
  env:
    GO_VERSION: oldstable
    GOTESTSUM_VERSION: latest
    GOLANGCILINT_VERSION: latest
Setup go version spec oldstable
oldstable version resolved as 1.22.10
Found in cache @ C:\hostedtoolcache\windows\go\1.22.10\x64
Added go to the path
Successfully set up Go version oldstable
go version go1.22.10 windows/amd64

[thaJeztah]

Opened a PR for those linting issues;

• fix linting issues #325

(not sure about the panic, because I'm not on Windows, so maybe it was just a incident; we'll see on the other PR)

[mat007]

I've been doing some testing/thinking on this. Overall I think it looks good, just have a few pieces of feedback.

Thanks! I’ll look into addressing these.

[mat007]

@kevpar can you please take another look?
I haven’t tried to fix the lint and go generate failures as they don’t appear to be linked to the changes at hand, and @thaJeztah has opened a separate pull request (thanks @thaJeztah!).

[kevpar]

@kevpar can you please take another look? I haven’t tried to fix the lint and go generate failures as they don’t appear to be linked to the changes at hand, and @thaJeztah has opened a separate pull request (thanks @thaJeztah!).

Changes look good, thanks! One thing that just occurred to me -- there is a chance of a panic if two goroutines try to close a listener at the same time (such that they both try to close closeCh). Should we wrap this in a sync.Once or something to prevent this?

[mat007]

One thing that just occurred to me -- there is a chance of a panic if two goroutines try to close a listener at the same time (such that they both try to close closeCh). Should we wrap this in a sync.Once or something to prevent this?

Good catch! This makes the implementation of Close much simpler as well.

[kevpar]

LGTM. Thanks for the change :)

[mat007]

@kevpar here is a PR to fix the panic with go generate: #326

[sorindumitru]

@kevpar We've also been running into this issue quite often as well, mostly in CI (feels like a quarter of the builds fail at least once because of this). Would it be possible to get a release out with the 3 changes from @mat007 (this one, #325 and #326).

[mat007]

@kevpar @helsaawy @kiashok what can I do to help move this fix forward?

[sorindumitru]

@kevpar @helsaawy @kiashok, pinging again here. We'd really like to have a new release with this fix merged in, it's causing our CI to break very frequently. Are there any plans to look into this PR some more? Or maybe more importantly, is the library still maintained?

[mat007]

@kiashok sorry for pinging you again.
Is there any chance we can merge this?

[sorindumitru]

@helsaawy, @ambarve I see that the linting issues were fixed in #334, maybe all that these needs now is to rebase it on top of main?

[mat007]

I’ve rebased the PR, can someone approve the workflow runs?
@kevpar @helsaawy @kiashok @ambarve ?