Repository navigation
Conversation
Since db18a31, writeClose returned the marshal error before writing, so Close with an invalid code or reason sent no close frame and the peer saw an abnormal closure. Close now sends StatusInternalError, completes the handshake and still returns the error, as v1.8.7 to v1.8.10 did.
A reason over the 123 bytes a close frame can carry made Close replace the caller's code with StatusInternalError and drop the reason, so the peer lost both. Close now sends the code with the longest prefix that does not split a UTF-8 rune and reports ErrCloseReasonTruncated.
The browser's close() throws for a reason over 123 UTF-8 bytes before changing any state, so Close returned an error and left the connection open. Close now truncates the reason as the native implementation does and records the reason it actually sent.
The previous wording promised that Close always closes the connection, but a concurrent Close can return after waitGoroutines times out while the first call is still blocked in the handshake. Only the first call is guaranteed to return with the connection closed.
ErrCloseReasonTruncated is reported even when writing the close frame fails, so its doc and the "sent N of M bytes" detail could be false. Both now describe only the shortening of the reason.
Go strings become JavaScript strings with invalid bytes replaced by U+FFFD, so a 50 byte reason of invalid UTF-8 already exceeds the browser's 123 byte limit. Truncation only guarantees a fit for valid UTF-8, and the library does not validate reasons.
U+FFFD decodes as utf8.RuneError with size 3, so a check on the rune alone would split it. Close must also report the transport error alongside ErrCloseReasonTruncated when the close frame write fails.
RFC 6455 defines the close reason as UTF-8, and peers that check it fail the connection instead of reading the code. On js/wasm, invalid bytes also grow when converted to a JavaScript string, so the browser threw for reasons that fit. Truncation now applies to the replaced reason, which can be longer than the caller's.
The caveat that invalid UTF-8 can still make the browser reject the reason no longer holds, since Close now replaces it before truncating. The doc now matches the native Close.
The error said "from" the caller's length, but replacing invalid UTF-8 can grow a reason, so the message could read "truncated from 122 to 123 bytes". Both numbers now describe the reason that was cut, so "from" is always larger than "to".
Close joined the truncation error first, so the message led with how the reason changed and the failure itself followed on the next line. On WASM, a close() that throws also dropped the truncation error the Close doc promises.
errors.go holds the exported errors. The helper and its limit move to close_shared.go, which has no build tag because both the native Close in close.go (!js) and the WASM Close in ws_js.go call it.
master defines maxCloseReason as maxControlPayload - 2, but the shared helper needs it on js, where frame.go is not built, so it became a literal 123. maxControlPayload moves to close_shared.go so both builds can derive the limit from it.
The unexported helpers and the tests restated what the Close doc, the code or the test names already say, and the RFC note in wireCloseReason was hard to follow. Keep each fact in the exported docs and only the comments that explain something the code does not show.
Member
Author
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 2 | Last posted: Round 2, 24 findings (11 P3, 5 P4, 8 Nit), COMMENT. Review Finding inventoryFinding inventory: PR #583Findings
Contested and acknowledgedCRF-1 (P3, close.go:182) - writeClose normalization has no test
CRF-18 (Nit, close_shared.go:1) -
|
| Reviewer | Focus |
|---|---|
| Bisky | tests |
| Chopper | ops/errors |
| Churn-guard | change verification |
| Ging | language modernization |
| Gon | naming |
| Hisoka | edge cases |
| Killua | perf |
| Kite | change integrity |
| Knov | contracts |
| Knuckle | SQL |
| Komugi | flake/determinism |
| Kurapika | security |
| Law | decomposition |
| Leorio | docs |
| Luffy | product |
| Mafu-san | process |
| Mafuuu | contracts |
| Melody | dispatch/pairing |
| Meruem | structural |
| Nami | frontend |
| Netero | mechanical checks |
| Pariston | premise testing |
| Pen-botter | product gaps |
| Razor | verification |
| Robin | duplication |
| Ryosuke | Go arch |
| Takumi | concurrency |
| Zoro | shape |
🤖 Managed by Coder Agents.
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR makes Close always send a close frame by normalizing the code and reason before the frame is built, on native and WASM. 9 P3, 2 P4, 7 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
ws_js.go:268: WASMClosepasses the code to the browser unchanged, includingCloseNow's 1001 (ws_js.go:248); the WHATWGclose()throws for codes other than 1000 and 3000-4999, soClosereturns an error and the connection stays open aftersetCloseErrrecorded a sent close (inferred from the spec, not run in a browser).read.go:271:parseClosePayloadaccepts a peer reason with invalid UTF-8, which RFC 6455 sections 5.5.1 and 8.1 say must fail the connection;Readreturns the raw bytes to the caller.close.go:92: "Additional calls to Close are no-ops" is inaccurate; a second call returnsfailed to close WebSocket: use of closed network connection.close.go:63: theCloseErrorandCloseStatusdocs and code use thevar ce; errors.Aspattern, and tests keeptc := tccopies, although go.mod declares go 1.26.
🤖 This review was automatically generated with Coder Agents.
The previous cleanup also removed comments that state facts the code does not show: why writeClose ignores the wireCloseError error, why wireErr is joined after the net.ErrClosed check and in that order, what wireCloseError guarantees, and why the CloseRead test exists. The truncated test field is renamed to wantErrSuffix because it holds the expected end of the error message.
Replacing invalid UTF-8 with U+FFFD can make a reason longer, so a 122-byte reason could report "truncated from 244 to 123 bytes". The error now ends with "after replacing invalid UTF-8" when the replaced length differs from the caller's.
Close joined the truncation error with any close failure and prefixed it with "failed to close WebSocket", so errors.Is could not tell a completed close with a shortened reason from a failed close. Close now returns the close failure if there is one, and otherwise the unprefixed wire error, so a match on ErrCloseReasonTruncated means the close completed. The invalid code error states that 1011 was sent, the Close doc lists valid codes and StatusNoStatusRcvd, and a test covers a long reason passed to writeError.
Member
Author
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 of the close-frame fix. 3 P3, 3 P4, 1 Nit, all new. The round 1 fixes hold, except that the comment added on the discarded writeClose error is false (reopened on that thread). The two contested items (no test for the peer echo, the close_shared.go name) are accepted.
The new findings are about the round 2 restructure of Close: the invalid-code error is now dropped when the close also fails, and the new writeClose and wireCloseError comments describe behavior the code does not have.
The CI test job failed on b8f74f0 after 2m36s with exit code 2; round 1's head and the base passed. The failure did not reproduce locally: go test -race ./..., the ci/test.sh command, CI=1 runs that include TestWasm in headless Chrome, and hundreds of runs of the new subtests all passed. The job log needs authenticated access. Read it or rerun the job; if a new subtest failed, that is a finding against this PR.
Out of scope (needs a ticket or explicit acceptance by a human):
close.go:128:exportedClosemaps everynet.ErrClosedto nil, soClosereturns nil when the connection was already gone and no close frame was sent.
🤖 This review was automatically generated with Coder Agents.
bytes encoded StatusNoStatusRcvd as a 1005 status code, which peers reject, and only a check in writeClose kept it off the wire, so the wireCloseError doc was wrong. bytes now returns no payload for it, and the writeClose comment states which callers drop the wire error. Tests cover Close with StatusNoStatusRcvd and print the peer's error when no close frame arrives; the CloseRead subtest is removed because other tests cover the net.ErrClosed path.
"A non-nil error reports a problem during the close" repeats the line before it, and a truncation-only error is not a problem during the close.
Member
Author
|
In reply to CRF-28 (P3, Agreed, the comment was wrong. ec299b6 replaces it: Close reports the error from its own wireCloseError call, and writeError and the reply to a peer's close frame drop it.
|
This branch has not been deployed
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
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.
Closenow sends a valid close frame for any code and reason. Since v1.8.11 (db18a31), a reason over 123 bytes or an invalid status code madeClosereturn an error without writing one, so the peer received no close code (browsers report 1006).Closereturns an error wrapping the newErrCloseReasonTruncated.Closereturns an error.On WASM,
Closehandles the reason the same way; there, the browser'sclose()would otherwise throw on a long reason and leave the connection open. The nativeClosedoc now states that the connection is closed when the firstClosereturns, even if it returns an error.The new connection tests fail on master and pass here, and the WASM tests pass in headless Chrome.
Decision log
Closereturns nil for a shortened reason.errors.Is(err, ErrCloseReasonTruncated)meansClosehad no other error to report. With both errors joined, a caller that skipsErrCloseReasonTruncatedwould also skip a failed close.