Repository navigation
Conversation
b973a9f to
a2fa86f
Compare
When the server answered 101 but a header check failed, Dial read up to 1024 bytes from the response body before returning. After a 101 the body is the upgraded connection, so a silent server made Dial wait for the 3 s timer. Close it instead and return an empty body; other statuses still return the first 1024 bytes.
a2fa86f to
e4d658a
Compare
This comment has been minimized.
This comment has been minimized.
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 3 | Last posted: Round 3, 4 findings (1 P3, 3 P4), APPROVE. Review Finding inventoryFindings
Downgrade and drop reasoning (R1)
Downgrade and drop reasoning (R3)
Contested and acknowledgedNone. Round logRound 1Netero first pass: no findings, 1 Note (CRF-1). Law not run (effective +67). Full panel, round 1: ging-go, ryosuke, killua, melody (specialists: Go code, connection and HTTP body lifecycle), pariston, mafuuu (core), bisky (tests), gon, leorio (once-per-PR floor), chopper (wildcard). Size 10 exceeds the focused target of 6-8 because four specialists (ging-go, ryosuke, killua, melody) plus the Gon and Leorio floor were required. Orchestrator verified CRF-2: with the server writing "after upgrade" after the 101 headers, head passes and base fails. 1 P3, 3 P4 posted; 2 Notes and 5 out-of-scope items in body; 5 dropped. Reviewed against e1a21e5..e4d658a. Round 2 updateBLOCKED. CRF-7 silent (no doc change, no reply). CRF-2, CRF-4, CRF-6 author fixed, unverified by panel. No review. Reviewed against e1a21e5..d967226. Round 3Churn guard PROCEED (CRF-7 addressed in 1abc068). Fix round (only comments, doc, and test data changed since R1 panel; no files added, no signature changes). Netero: no findings; verified base dial.go fails the new test on the body assertion. Panel: bisky, gon, leorio (raised CRF-2, CRF-4, CRF-6, CRF-7), ging-go, ryosuke (.go files), chopper (wildcard). CRF-2, CRF-4, CRF-6, CRF-7 fixes verified by panel. Gon raised 3 P2 wording items and 1 Note, all dropped (CRF-17 to CRF-20). No open findings. Reviewed against e1a21e5..1abc068. 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 makes Dial close the connection and return http.NoBody instead of reading the body when the handshake fails after a 101 response. 1 P3 and 3 P4 findings.
Out of scope (needs a ticket or explicit acceptance by a human):
main_test.go:23: theTestMainleak check comparesruntime.NumGoroutine()with 1 right afterm.Run(), so a goroutine that is still exiting fails the package; on the base commitgo test -count=1 -race -run 'TestBadDials|TestDial' .failed 6 of 20 runs.dial.go:162: the 3 second timer that closes a stalled non-101 body has no test, so a regression there would make failed dials hang.dial.go:160:1024is an unnamed literal that theDialdoc repeats, so the two can drift.dial.go:162:time.Second*3is an unnamed literal, and the new test's 1 s limit depends on it.dial.go:159: the debug-read comment does not state the size or time limits.
Notes:
dial.go:152: with a customhttp.RoundTripperthat returns a 101 with a body that is not anio.ReadWriteCloser(as inTestBadDials/badBody), the body is an ordinary buffer, and this PR changes the returnedresp.Bodyfrom its bytes to empty.net/httpalways returns a read-write body for a 101.dial.go:154: removingrespBody.Close()passesTestDialFailedHandshakeBodyand fails only the package-level goroutine leak check inTestMain.
dial.go:113
P4 [CRF-7] The Dial doc still says callers can read the first 1024 bytes of the body on error, but after a failed 101 handshake resp.Body is always empty. (Leorio)
A caller who logs resp.Body while debugging an invalid Sec-WebSocket-Accept error gets "" and cannot tell from the doc whether the server sent nothing or Dial discarded the bytes. Suggested:
// If an error occurs, the returned response may be non nil.
// For a response with status 101 Switching Protocols, resp.Body is empty,
// because the bytes after a 101 belong to the upgraded connection.
// For any other status, resp.Body holds at most the first 1024 bytes of the body.🤖
🤖 This review was automatically generated with Coder Agents.
With a silent server, an empty body also results from reading the upgraded connection with a short timer. Bytes after the 101 make that variant fail on the body check.
The read is bounded by the 3 s timer, so "waits for data the server may never send" overstated it.
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.
The read also stalls when the server sends fewer than 1024 bytes and keeps the connection open, not only when it sends nothing.
The Dial doc said the body on error holds up to 1024 bytes, so a caller debugging a failed 101 could not tell an empty body from a silent server.
|
In reply to CRF-7 (P4, Fixed in 1abc068: the
|
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.
When the server answers
101 Switching Protocolsbut the handshake still fails, for example on an invalidSec-WebSocket-Acceptor a subprotocol the client did not offer,Dialnow closes the response body and returns at once.Previously, every
Dialthat failed after receiving a response read up to 1024 bytes of the response body for debugging, with a 3 second timer. After a 101 the body is the upgraded connection, not an HTTP body, so when the server sent nothing more, each of these failures took 3 seconds.For these failures the returned response keeps its status and headers, and
resp.Bodyis empty (http.NoBody). Failures with any other status are unchanged:resp.Bodyholds up to the first 1024 bytes, read with the same 3 second limit.The tradeoff: if a server answers 101 and then writes error text on the connection,
resp.Bodyno longer contains that text. Bytes after a 101 belong to the upgraded protocol, not to an HTTP body.TestDialFailedHandshakeBodycovers a 101 with an invalidSec-WebSocket-Acceptfrom a server that then sends a few bytes and keeps the connection open (fails on master after 3.0 s with those bytes in the body; passes with this change in about 0 s against a 1 s limit) and a 403 whose body is still returned. Lint, race tests and the WASM test pass.Decision log
http.NoBodyinstead of a nilresp.Body: whenDialreturns a response with an error,resp.Bodyhas never been nil, so callers that read it need no new nil check.Dialdoc now says the body is empty after a 101. Its old text ("you can only read the first 1024 bytes of the body") stayed literally true, but a caller debugging a failed 101 could not tell an empty body from a silent server.