Repository navigation
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 3 | Last posted: Round 3, 26 findings (11 P3, 6 P4, 9 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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
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.
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.
Close returns the invalid code error even when the connection was already closed and no close frame was written, so "sent StatusInternalError instead" could be false. It now says the code was replaced, as the truncation error only says the reason was shortened. The two helper docs now state the same contract in the same words.
mafredri
force-pushed
the
mafredri/close-reason-truncate
branch
from
October 26, 2026 15:32
ac78959 to
5fdd32d
Compare
mafredri
marked this pull request as ready for review
October 26, 2026 17:18
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.