Repository navigation
Conversation
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 4 | Last posted: Round 4, 15 findings (6 P3, 8 P4, 1 Nit), 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
CRF-7 (P3, dial.go:272) - badStatusError compatibility claim and its sources
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. Round 3Churn guard PROCEED: CRF-14, CRF-16, CRF-17 addressed; CRF-7 contested. Fix round (docs and tests; no files added; no exported signature change; title changed to feat! with a breaking-change paragraph but the stated goal is unchanged; additions 79 to 127). Netero: no findings. Panel: mafu-san, leorio, gon (contested CRF-7, previous agents reused), bisky (tests changed), ging-go, ryosuke (Go files changed), pariston (new breaking-change premise), wildcard zoro. Ging-go, Ryosuke: no findings. Round 4Churn guard PROCEED: CRF-18, CRF-19 addressed. Restructure round: Unwrap replaced by an Is method, the ErrBadStatus contract wording changed from "wraps" to "matches", docs moved from errors.go to Dial, and ws_js.go joined the diff. Netero: no findings; CRF-19 fix verified by mutation. Full panel (focused, target 6-8): ging-go, ryosuke (specialists), pariston, mafuuu (core), bisky (tests), gon, leorio (doc comments changed), wildcard meruem. Ging-go, Ryosuke, Pariston, Mafuuu, Bisky, Meruem: no findings; four reviewers reproduced that restoring Unwrap fails nonSwitchingStatus. 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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mafredri
changed the title
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mafredri
changed the title
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
The Dial doc repeated the condition that the ErrBadStatus doc states, so a change to the condition had to edit both.
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.
The doc said a redirect that CheckRedirect refuses never wraps ErrBadStatus, but returning http.ErrUseLastResponse stops the redirect and returns the 3xx response, which does wrap it.
The ErrBadStatus doc says how a CheckRedirect error and http.ErrUseLastResponse are reported, and a non-writable body after a 101 must not wrap it, but no test checked either.
An Is method would also satisfy errors.Is and would keep the innermost error's text. Unwrap is kept so code that walks the error chain finds ErrBadStatus too.
A single errors.Unwrap on the Dial error returns badStatusError, so the comment's errors.Unwrap claim held only for code that walks the chain.
The ErrBadStatus doc described which HTTPClient response counts, how CheckRedirect errors are returned and what happens on js/wasm, which is Dial's behavior. ErrBadStatus now says only what it means, and both Dial docs state their own rules.
Unwrap made ErrBadStatus the innermost error, so code that unwraps a Dial error to its innermost error got "websocket: bad status" instead of the status text. An Is method gives the same errors.Is result without changing the chain.
Unwrap and Is give the same errors.Is result, so only the innermost error shows whether a change brought back the old chain.
The helper returns an *http.Client, but HTTPClient: redirect(...) read as performing a redirect.
ErrBadStatus is new, so "for backward compatibility" alone did not say what is kept. The innermost error is still the status text.
b8ae92d to
72732d9
Compare
The error from
Dialnow matches a new sentinel error,ErrBadStatus, undererrors.Iswhen 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. The error does not matchErrBadStatusin 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 matchesErrBadStatusthrough anIsmethod. It does not unwrap toErrBadStatus, so code that unwraps the error to its innermost error still gets the status text.On js/wasm, the error from
Dialnever matchesErrBadStatus, because the browser does not expose the handshake response.New tests cover a 403 with a 1025-byte body, a round tripper that returns an error, and a 302 with a
CheckRedirecterror orhttp.ErrUseLastResponse.Test_verifyServerHandshakechecks that only its non-101 case matchesErrBadStatus, and the non-writable body test checks that its error does not. The 403 test fails on master with only the variable declared. It also checks the innermost error's text, which fails ifbadStatusErrorunwraps toErrBadStatusinstead of matching it throughIs. Lint, race tests and the WASM test pass.Refs #424, #528
Decision log
ErrBadStatusin golang.org/x/net/websocket.Ismethod instead ofUnwrap, so the innermost error keeps its text.errors.Isgives the same result either way, anderrors.Ascannot targetErrBadStatus, whose type is unexported. Only code that compareserrors.Unwrapresults withErrBadStatuscannot find it.%wprefix (websocket: bad status: expected ...), because it changeserr.Error(), which public projects compare in full.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.CheckRedirecterror other thanhttp.ErrUseLastResponsestays a request error with a nil response, becausenet/httpthen returns the previous response with its body closed.http.ErrUseLastResponsegives the redirect response, which matchesErrBadStatus.Dialstill closes the body.