Repository navigation
Conversation
Dial returned a non-nil response both when the server rejected the handshake with a non-101 status and when a 101 response failed validation, so callers checking resp != nil treated protocol violations as HTTP errors. Dial now wraps ErrBadStatus only for a non-101 status, and documents that the response and its first 1024 body bytes are available in that case.
Wrapping ErrBadStatus with %w inserted "websocket: bad status: " into the Dial error string, and public projects match on the existing text. An unexported error type now returns the previous text and unwraps to ErrBadStatus, so errors.Is still holds.
6a36d7a to
ddfc2e5
Compare
This comment has been minimized.
This comment has been minimized.
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 2 | Last posted: Round 2, 10 findings (5 P3, 5 P4), COMMENT. Review Finding inventoryFinding inventory: PR #587Findings
Contested and acknowledgedCRF-4 (P3, dial.go:274) - ErrBadStatus exposes no status code once the error is wrapped
Round logRound 1Netero first pass: 1 P3, 1 P4, 1 Note; no P0, panel proceeded. Round type: round 1, full panel. Focused (Go only), size target 6-8: ging-go, ryosuke (specialists), pariston, mafuuu (core), bisky (tests changed), gon, leorio (once-per-PR floor), mafu-san (agent-authored PR); wildcard knov (random from distilled set). 9 panel reviewers, one over target because mafu-san was added for the agent-authored PR. Ging-go, Ryosuke, Knov: no findings. Round 2Churn guard PROCEED: CRF-1, 2, 5, 6, 7, 8 addressed; CRF-4 contested. Fix round (docs and tests only, no files added, no exported signature change, additions fell from 102 to 79). Netero: no findings, CRF-1 and CRF-2 fixes verified. Panel: pariston (contested CRF-4, previous agent reused), mafuuu, bisky, gon, leorio, mafu-san (raised findings on touched code; gon and leorio also for doc-comment changes), ging-go, ryosuke (Go files changed), wildcard knov. Ging-go, Ryosuke, Knov, Mafuuu: no findings. 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.
This PR adds ErrBadStatus, which the Dial error wraps when the handshake response status is not 101. 4 P3 and 3 P4 findings.
Out of scope (needs a ticket or explicit acceptance by a human):
dial.go:88-100:cloneWithDefaultsalways installs aCheckRedirectthat returns nil when the caller set none, which removes thenet/http10-redirect limit, so a server that redirects to itself makesDialloop until the context ends.dial.go:151-165: after a 101 response fails validation,Dialreads from the upgraded connection and blocks for 3 seconds when the server sends nothing (measured 3.014s). The decision log calls this a separate change, and no ticket is linked.dial.go:154,dial.go:156: the 1024-byte body limit and the 3-second body read timeout are unnamed literals, and theDialdoc does not mention the 3-second wait.dial.go:117: "This function requires at least Go 1.12" is stale becausego.moddeclaresgo 1.26, so a reader may take it as a current constraint.
🤖 This review was automatically generated with Coder Agents.
Dial truncates the body of every response it returns with an error, not only for ErrBadStatus, and the previous doc stated the limit only for a non-101 status. The ErrBadStatus condition is now stated once, in the ErrBadStatus doc, so the Dial doc says only what the caller gets.
A 302 refused by HTTPClient.CheckRedirect makes HTTPClient.Do return an error, so Dial returns a request error with a nil response and no ErrBadStatus. The doc said any non-101 answer wraps ErrBadStatus, which is false for that redirect.
The comment claimed that callers match on the Dial error string, which no linked caller supports, and described the history of the change. It now states only how Error and Unwrap behave.
invalidAccept checked one of the six 101 validation failures, so making another one wrap ErrBadStatus left the tests passing. The assertion in Test_verifyServerHandshake covers every row and replaces invalidAccept.
Dialing the address of a closed httptest.Server can reach a server that a parallel test started on the same port. A round tripper that returns an error tests the same nil-response path without a port.
Public callers match on the Dial error text, for example with strings.EqualFold on the full message, so the reason the type exists belongs in its comment.
|
/coder-agents-review
|
There was a problem hiding this comment.
This round reviews the ErrBadStatus doc and test changes made after the first review: 1 new P3, 2 new P4, and 1 P3 raised again.
Fixed: the 1024-byte body limit in the Dial doc, the condition repeated in two docs, the refused-redirect behavior for non-ErrUseLastResponse errors, the per-failure ErrBadStatus check in Test_verifyServerHandshake, and the networkError port dependency. The status code question is closed by the maintainer decision. The out-of-scope items listed in the first review still have no ticket.
🤖 This review was automatically generated with Coder Agents.
| @@ -123,6 +124,54 @@ func TestBadDials(t *testing.T) { | |||
| }) | |||
There was a problem hiding this comment.
P4 [CRF-16] TestBadDials/badBody checks only the error text, so nothing fails if the non-writable body error, which the PR description lists as not wrapping ErrBadStatus, starts wrapping it. (Bisky)
That error is built in dial (dial.go:173), outside verifyServerResponse, so the new assertion in Test_verifyServerHandshake does not see it. Changing it to fmt.Errorf("response body is not a io.ReadWriteCloser: %T: %w", respBody, ErrBadStatus) leaves go test -count=1 . passing. Add assert.Equal(t, "errors.Is(err, ErrBadStatus)", false, errors.Is(err, websocket.ErrBadStatus)) after the assert.Contains.
🤖
| // ErrBadStatus is wrapped by the error from Dial when the final response | ||
| // that DialOptions.HTTPClient returns for the opening handshake has a | ||
| // status other than 101 Switching Protocols. See Dial for the response | ||
| // returned with it. A redirect that HTTPClient.CheckRedirect refuses is |
There was a problem hiding this comment.
P4 [CRF-17] No test covers the redirect behavior that the ErrBadStatus doc now states. (Mafu-san)
If handshakeRequest changes how a refused redirect is returned, this doc becomes false and every test still passes. A transport that returns 302 reproduces both cases: a CheckRedirect error gives a request error with a nil response, and http.ErrUseLastResponse gives ErrBadStatus with the 302 response. Add both as subtests of TestDialErrBadStatus, using the transport pattern from TestDialRedirect (dial_test.go:361).
🤖
Dialnow wraps a new sentinel error,ErrBadStatus, when the server answers the opening handshake with a status other than 101 Switching Protocols. Aftererrors.Is(err, websocket.ErrBadStatus), the returned response is non-nil andresp.Bodyholds up to the first 1024 bytes of the response body.Before this change, the only signal was a non-nil response, as advised in #424. But
Dialalso returns a non-nil response when a 101 response fails validation: an invalidConnection,UpgradeorSec-WebSocket-Acceptheader, an unexpected subprotocol or extension, or a body that is not writable. A caller that treats every non-nil response as an HTTP rejection reports those protocol failures as rejections.ErrBadStatusis not wrapped in those cases, or when no response arrives.The error text is unchanged on purpose, for example
failed to WebSocket dial: expected handshake response status code 101 but got 403, because public projects match on it. An unexported error type returns that text and unwraps toErrBadStatus.On js/wasm,
Dialnever wrapsErrBadStatus, because the browser does not expose the handshake response.New tests cover a 403 with a 1025-byte body and a round tripper that returns an error, and
Test_verifyServerHandshakechecks that only its non-101 case wrapsErrBadStatus. The 403 test fails on master with only the variable declared. Lint, race tests and the WASM test pass.Refs #424, #528
Decision log
ErrBadStatusin golang.org/x/net/websocket.Unwrapinstead of anIsmethod, soErrBadStatusis in the error chain that the docs describe as wrapped.%wprefix (websocket: bad status: expected ...), because it changes text that public projects match on.Dialreturnsresp, and a caller that needs the status after wrapping the error can wrap it with the status. It can be added later if someone asks.*http.Response, because its body is the same reader as the returned response's body, so one read empties both.resp.StatusCode != 101check only, because it gives no condition thaterrors.Iscan test after a caller wraps the error.HTTPClient.CheckRedirectrefuses stays a request error with a nil response, becausenet/httpthen returns the previous response with its body closed. Returninghttp.ErrUseLastResponsefromCheckRedirectgives the redirect response, which wrapsErrBadStatus.Dialstill closes the body.