pipe: make consumed listener close authoritative - #369
Open
Devon Krisman (dkrisman) wants to merge 1 commit into
Open
pipe: make consumed listener close authoritative#369Devon Krisman (dkrisman) wants to merge 1 commit into
Devon Krisman (dkrisman) wants to merge 1 commit into
Conversation
Once makeConnectedServerPipe receives from closeCh, always return ErrPipeListenerClosed. ConnectNamedPipe can race the handle close and report ERROR_PIPE_CONNECTED (normalized to nil by connectPipe) or ERROR_NO_DATA instead of ErrFileClosed. The old close branch reused that raced result, so listenerRoutine kept running after the single close signal was spent and win32PipeListener.Close waited forever. Add a regression test that races a client connect against the close signal, and a stress test racing Accept with Close. Fixes microsoft#85 Signed-off-by: Devon Krisman <winio@krisman.dev>
Author
|
@microsoft-github-policy-service agree |
Trustin Lee (trustin)
added a commit
to leapmux/leapmux
that referenced
this pull request
Aug 7, 2026
…366) ## Motivation The Windows CI job timed out after 10 minutes with the Hub parked in shutdown (https://github.com/leapmux/leapmux/actions/runs/31161153380/job/92811556092). The goroutine dump names the mechanism. go-winio's `win32PipeListener.Close` sends ONE token on the listener's private `closeCh` and then waits for `doneCh`. An accept that is already in flight takes that token first, inside `makeConnectedServerPipe`, and that path maps the aborted connect to `ErrPipeListenerClosed` only when the connect returned `nil` or `ErrFileClosed`. Any other result -- the `ERROR_NO_DATA` that `locallisten.WaitReady`'s connect-and-close probe leaves behind, for example -- goes back to the listener routine, which reads `closed = err == ErrPipeListenerClosed` as false and keeps running. `doneCh` stays open and the close never returns. That is microsoft/go-winio#85, open since 2018 and unfixed in v0.6.2, the newest release; its fix, microsoft/go-winio#369, is open and unmerged. The hang does not stay inside winio. `http.Server.Shutdown` closes its listeners while it holds `srv.mu`, so `Serve` cannot return, no connection can finish, and `hub.Serve` waits forever for a listener result. Solo's `join` never sees `hubDone`. `Shutdown`'s own context deadline cannot save it, because it blocks before it reads the context. The desktop sidecar runs the same path, so a real shutdown can wedge the same way -- this is not only a test problem. The wedge was seen before and worked around by deleting coverage: `TestDialer_NpipeAcceptsFullNTPath` carried a comment describing this exact goroutine state and had been cut down to a parser-only check. ## Modifications - `npipeListener.Close` retries the winio close every 250 ms, up to 8 times. A later token stops the listener routine from either state it parks in: its own select receives the token, or `makeConnectedServerPipe` receives it and aborts a connect that no client satisfied, which does map to `ErrPipeListenerClosed`. - The retry is bounded and reports `errCloseStuck` rather than wait a listener out. Returning is the point: a `Close` that blocks holds `srv.mu` and deadlocks the whole `http.Server`. The bound also covers microsoft/go-winio#357, a hang no extra token can release. The cost of giving up is one pipe name that stays taken, which the error names. - `newNpipeListener` becomes the one construction site, so a listener a test builds carries the same close policy as a production one. - `TestDialer_NpipeAcceptsFullNTPath` gets its round trip back. - New tests cover the retry, the bounded give-up, the error the listener reports, a double close (the Hub closes its local listener twice), concurrent closes, and the race against a real pipe. No breaking change and no deprecation. The behavior is Windows-only; the Unix listener is untouched. ## Result - A Hub or desktop sidecar shutdown on Windows no longer deadlocks when a listener close races an accept that is in flight. `Shutdown` returns, `Serve` returns, and solo's `Stop` completes. - A listener that cannot be closed at all is reported to the caller instead of blocking it forever, so the failure reads as a named error in the teardown path rather than as a 10-minute CI timeout with no message.
|
Independent downstream confirmation on Windows with go-winio v0.6.2: a watchdog/restart race test hung with Close waiting on doneCh and listenerRoutine back in its top-level select. This supports the consumed-close failure described in #85, rather than proving an outstanding overlapped operation never completes. We tested the equivalent change locally: after consuming closeCh, close the pending pipe, drain the connect result, and unconditionally return ErrPipeListenerClosed. With that patch, 100 race-enabled watchdog/restart repetitions passed, followed by our full downstream race suite and go vet. The unpatched focused repetition reproduced the hang. We intend to carry a pinned local patch while this is reviewed. This is independent downstream validation of the production change, not a claim that we have run this PR's added tests or that it resolves every shutdown failure described in #357. Thanks for submitting the focused fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #85. Once
makeConnectedServerPipeconsumes the listener's close signal,treat closure as authoritative and always return
ErrPipeListenerClosed,instead of reusing a raced
ConnectNamedPiperesult.Problem
win32PipeListener.Close()sends a single value onl.closeCh. WhenmakeConnectedServerPipereceives it, the current code closes the pending pipehandle and then reuses the connect goroutine's result, converting only
niland
ErrFileClosedtoErrPipeListenerClosed.Closing the handle does not guarantee the connect result is one of those two. A
client that attaches in the window before
ConnectNamedPipeis called makes itreturn synchronously:
ERROR_PIPE_CONNECTED(whichconnectPipenormalizes tonil) if the client is still attached, orERROR_NO_DATAif it alreadydisconnected. In the
ERROR_NO_DATAcase the error is passed through,listenerRoutine's retry loop callsmakeConnectedServerPipeagain, and theone close signal has already been spent: the new connect pends forever and
Close()waits forever onl.doneCh.Solution
Once
makeConnectedServerPipereceives froml.closeCh, that close isauthoritative. It still closes the pending pipe handle and drains the connect
result so the goroutine completes, but always returns
ErrPipeListenerClosed, which stopslistenerRoutine.Two regression tests:
TestListenerCloseOverridesRacedConnectdrivesmakeConnectedServerPipedirectly: a raw client attaches and disconnects in the pre-connect window
(reliably producing the synchronous
ERROR_NO_DATA), then a close signalraces the connect result. It fails within a few iterations on the current
code and passes with this change.
TestListenerCloseRacesPendingConnectracesAccept, a dialing client, andListener.Closeend to end, with bounded timeouts so a regression failsinstead of hanging the test process.
Testing
go test -race -count=100 -run 'Test.*Listener.*Close' .go test -race .golangci-lint run --config=.golangci.ymlclean (v1.64.8)go generate ./...with no generated diffTestListenerCloseOverridesRacedConnectfails on unpatchedmainwith
expected ErrPipeListenerClosed, got The pipe is being closed.Related work
PR #324 also touches listener shutdown synchronization more broadly (PR #364
did as well before it was closed). This patch is limited to the specific
lost-close race from #85: the goroutine that consumes
closeChmust not let araced connect result override listener closure.