Repository navigation
Conversation
b76fd5d to
e5aa423
Compare
|
/coder-agents-review |
|
Review posted | Chat Review history
deep-review v0.5.0 | Round 1 | Last posted: Round 1, 8 findings (1 P2, 3 P3, 2 Nit, 2 Note), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel. 1 P2, 3 P3, 2 Nit, 2 Note new. 1 pre-existing Note dropped. Reviewed against d099e16..e5aa423. Panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Ging-Go, Gon, Leorio, Takumi, Meruem, Killua, Chopper. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean diagnosis, proportional fix. The userClosed atomic correctly separates user intent from internal close coordination, and both Close and CloseNow are updated symmetrically. The commit message is the standout: it names the condition, the mechanism, and the fix, then enumerates all three behavioral states.
The dominant finding is the missing regression test. Nine of twelve reviewers flagged it independently. The race this PR fixes (internal read loop wins casClosing before the user calls Close) has no test coverage, and no existing test distinguishes the old behavior (wrapped net.ErrClosed) from the new behavior (nil). A bug fix without a test for the bug is incomplete work.
Two doc comments need attention: Close's comment claims repeated calls are "no-ops" (they return net.ErrClosed post-close), and CloseNow's comment is silent on the topic entirely.
Severity breakdown: 1 P2, 3 P3, 2 Nit, 2 Note.
"I tried to build a simpler alternative using only isClosed() as the early guard, but it returns net.ErrClosed on a first user call when the internal close already completed, which is wrong from the user's perspective: their first Close should succeed regardless of internal state. The userClosed separation is the minimal mechanism that preserves that invariant." (Pariston)
close.go:92
P3 [CRF-1] Doc comment claims "Additional calls to Close are no-ops" but the new behavior returns net.ErrClosed after the connection is fully closed. The three behaviors in the commit message (first call nil, concurrent calls nil, post-close net.ErrClosed) are the actual contract; the doc should match. (Netero, Pariston)
🤖
close.go:130
P3 [CRF-4] CloseNow's doc comment says nothing about repeated or concurrent call behavior. The three behaviors in the commit message apply to CloseNow identically, but someone writing defer c.CloseNow() that might race with another goroutine has no way to know from the godoc what happens. (Leorio)
🤖
🤖 This review was automatically generated with Coder Agents.
| if err != nil { | ||
| return err | ||
| } | ||
| if c.userClosed.Swap(true) && c.isClosed() { |
There was a problem hiding this comment.
P2 [CRF-3] No regression test for the race condition this PR fixes. The bug: when the internal read loop (read.go:361) or write path (write.go:365) wins casClosing() before the user calls Close, the old code returned a wrapped net.ErrClosed. No existing test exercises this interleaving.
The existing
TestConcurrentClosePingexercises concurrent ping+close but not any close+close path.TestConnClosePropagationexercises close propagation but not the return value contract for the caller that loses the casClosing race. (Mafuuu)
I verified this by reverting the PR changes and running all close-related tests 10 times with the race detector on the old code. All passed. (Pariston)
A test that reproduces this: set up a connection with CloseRead, trigger the peer to send a close frame so the local read loop wins casClosing, then call Close from user code and assert nil return. (Bisky P1, Hisoka P2, Mafu-san P2, Mafuuu P2, Meruem P2, Chopper P2, Pariston P3, Leorio P3, Takumi P3)
🤖
| } | ||
| }() | ||
|
|
||
| if c.casClosing() { |
There was a problem hiding this comment.
Nit [CRF-5] The PR description claims concurrent user calls "return instantly without blocking." In practice, a concurrent caller reaches casClosing() -> true, enters waitGoroutines(), and blocks on c.closeReadDone and c.closed with a 15-second timer. The blocking is correct behavior (caller gets confirmation that close completed), but the description overstates it. (Pariston, Mafuuu, Takumi)
🤖
| closing atomic.Bool | ||
| closeMu sync.Mutex // Protects following. | ||
| closed chan struct{} | ||
| userClosed atomic.Bool // Set by Close/CloseNow on first user call. |
There was a problem hiding this comment.
P3 [CRF-6] userClosed reads as completion alongside closed, but it tracks initiation. The struct now has three close fields in near-identical tenses (userClosed, closing, closed) where the first means "called" and the last means "done." Also, CloseRead's goroutine (read.go:82) calls the public c.Close(), setting userClosed from internal library code. closeRequested would disambiguate stage and scope without reading the body. (Gon P3, Hisoka Note, Chopper Note)
🤖
| closing atomic.Bool | ||
| closeMu sync.Mutex // Protects following. | ||
| closed chan struct{} | ||
| userClosed atomic.Bool // Set by Close/CloseNow on first user call. |
There was a problem hiding this comment.
Nit [CRF-7] Comment "on first user call" restates what the name and type already show. The user is in the name, first call is derivable from atomic.Bool set once. Trim to // Set by Close/CloseNow. or // Tracks whether the user has called Close/CloseNow. (Gon, Leorio)
🤖
| closing atomic.Bool | ||
| closeMu sync.Mutex // Protects following. | ||
| closed chan struct{} | ||
| userClosed atomic.Bool // Set by Close/CloseNow on first user call. |
There was a problem hiding this comment.
Note [CRF-8] Close state is now tracked across three independent boolean-ish variables (userClosed, closing, closed channel) forming an implicit state machine with ~5 reachable states and undocumented transitions. The current approach works correctly and is proportionate for a bugfix, but the next person who modifies close coordination must understand all three in combination. (Meruem)
🤖
| if err != nil { | ||
| return err | ||
| } | ||
| if c.userClosed.Swap(true) && c.isClosed() { |
There was a problem hiding this comment.
Note [CRF-9] The userClosed.Swap(true) && c.isClosed() check is not atomic (the connection could finish closing between the swap and the channel probe), but the gap is benign. A second user call that falls through instead of returning net.ErrClosed immediately reaches casClosing() -> waitGoroutines(), the defer suppresses net.ErrClosed, and the caller gets nil. The system converges to the correct state on both sides of the gap. (Takumi)
🤖
Close and CloseNow returned a wrapped net.ErrClosed when the internal read loop or write path won the casClosing race before the user-facing call. The defer that suppresses net.ErrClosed was only registered on the CAS winner path. Separate user-facing close tracking from internal close coordination. A userClosed atomic tracks whether the user has called Close/CloseNow before. Combined with isClosed, this gives three distinct behaviors: - First user call, close in progress or peer closed: nil. - Concurrent user calls while close is in progress: nil, returns instantly without blocking. - User call after close has fully completed: net.ErrClosed.
When the read or write path closed the connection first, for example after the peer's close frame, the user's first Close or CloseNow returned a wrapped net.ErrClosed. The first user call now returns nil in that case, and library code that closes the connection (CloseRead and NetConn reads) uses an internal close so it does not count as that call.
On Wasm, the user's first Close returned a wrapped net.ErrClosed after the browser or library code had already closed the connection, the same failure as the native fix. Close now applies the same first-call rule, and the read limit, read timeout, CloseRead and finalizer paths use an internal close. TestWasm gains a server mode that closes first.
e5aa423 to
21b8e2a
Compare
mafredri
changed the title
The first call to
CloseorCloseNowcould returnfailed to close WebSocket: use of closed network connectioneven when the connection had closed normally. This happens when the library closes the connection before the user callsClose: after the peer's close frame is read, whenCloseReadreceives a data message, or when aNetConnreads a message of the wrong type. On Wasm, the same error appeared after the browser closed the connection. Code that reads until a close error and then callsClosetherefore failed on every normal peer close.Now the first user call to
CloseorCloseNowreturns nil in these cases. It still waits until the connection, and theCloseReadgoroutine if any, have finished closing. Later calls behave as before: they wait, then returnnet.ErrClosed.Library code that closes the connection now calls an internal function instead of
Close, so it does not use up the user's first call. On Wasm this covers the error event, failed writes, the read limit, the read timeout,CloseReadand the finalizer.TestConn/firstCloseAfterLibraryCloseandTestWasmFirstCloseAfterPeerClosefail on master with the error above and pass on this branch.go test -race ./...andTestWasmpass.This branch conflicts with #583 in both
Closebodies (native and Wasm). Whichever PR merges second needs a rebase.Refs coder/internal#1541
Decision log
net.ErrClosed, because repeated-Closebehavior is a separate decision.net.ErrClosedafter a library close finished.Close, they would use up the user's first call and the user's ownClosewould still fail; thecloseReadDataMessageandnetConnUnexpectedTypesubtests cover this.CloseReaddefers the internal close withStatusGoingAway, which is what WasmCloseNowsends.Closedecision.closeReadDataMessagesubtests take 15s because theCloseReadgoroutine waits for its own exit until the 15s timeout. Master has the same wait; this PR does not change it.TestWasmserver closes first when the client requests theclosesubprotocol.