Fix deadlock when closing pipe

[Mathieu Champlon (mat007)]

Hi!

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

Thanks!

[Sebastiaan van Stijn (thaJeztah)]

Mathieu Champlon (@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 😂)

[Mathieu Champlon (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. 🤔

[Sebastiaan van Stijn (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

[Kevin Parsons (kevpar)]

Mathieu Champlon (@mat007) can you explain what the deadlock is that you're seeing?

[Mathieu Champlon (mat007)]

Mathieu Champlon (@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.

[Mathieu Champlon (mat007)]

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

[Kevin Parsons (kevpar)]

Kevin Parsons (@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!

[Kevin Parsons (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!

[Kevin Parsons (kevpar)]

Also, looks like there are some CI failures.

[Sebastiaan van Stijn (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

[Sebastiaan van Stijn (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)

[Mathieu Champlon (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.

[Mathieu Champlon (mat007)]

Kevin Parsons (@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 Sebastiaan van Stijn (@thaJeztah) has opened a separate pull request (thanks Sebastiaan van Stijn (@thaJeztah)!).

[Kevin Parsons (kevpar)]

Kevin Parsons (@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 Sebastiaan van Stijn (@thaJeztah) has opened a separate pull request (thanks Sebastiaan van Stijn (@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?

[Mathieu Champlon (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.

[Kevin Parsons (kevpar)]

LGTM. Thanks for the change :)

[Mathieu Champlon (mat007)]

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

[Sorin Dumitru (sorindumitru)]

Kevin Parsons (@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 Mathieu Champlon (@mat007) (this one, #325 and #326).

[Mathieu Champlon (mat007)]

Kevin Parsons (@kevpar) Hamza El-Saawy (@helsaawy) Kirtana Ashok (@kiashok) what can I do to help move this fix forward?

[Sorin Dumitru (sorindumitru)]

Kevin Parsons (@kevpar) Hamza El-Saawy (@helsaawy) Kirtana Ashok (@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?

[Mathieu Champlon (mat007)]

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

[Sorin Dumitru (sorindumitru)]

Hamza El-Saawy (@helsaawy), Amit Barve (@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?

[Mathieu Champlon (mat007)]

I’ve rebased the PR, can someone approve the workflow runs?
Kevin Parsons (@kevpar) Hamza El-Saawy (@helsaawy) Kirtana Ashok (@kiashok) Amit Barve (@ambarve) ?