Repository navigation
fix(http): default reason phrase; root the body chunk across a replaced writeHead - #13722
Merged
Merged
Conversation
…te chunk across implicit_header Two causes in perry-ext-http's server response: 1. A statusMessage of "" (Next's send-response copies Response.statusText, which is "") was never defaulted at head commit, so the wire got "HTTP/1.1 200 ". The native writeHead and implicit_header now apply node's statusMessage ||= STATUS_CODES[code] || 'unknown' (ResponseState::commit_status_message), and writeHead(code, "") keeps an explicit empty reason as node does. 2. finalize_buffered_end and js_node_http_res_write_with_cb read the body chunk after implicit_header, which can run a program-replaced writeHead and a GC. A moved young chunk left a forwarding pointer, so the body was read as empty or truncated while Content-Length was already sent and the client hung (first reused keep-alive request on a fresh Next server). The chunk is now rooted before implicit_header and read back after it.
|
Caution Review failedThe pull request is closed.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
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.
Two perry
node:httpserver bugs found by profiling compiled Next.js (secret-tests/nextjs-hello) against node.A. Empty reason phrase
Next sets
res.statusMessage = ""(send-response.js:22). On native responses nothing applied node's default (statusMessage ||= STATUS_CODES[statusCode]at head commit), so/api/helloansweredHTTP/1.1 200instead ofHTTP/1.1 200 OK. The newcommit_status_messageinperry-ext-http/src/server/response.rsruns fromwriteHeadand the implicit header. An explicitres.writeHead(200, "")keeps the empty reason, as node does.B. Response body lost or truncated after a replaced
writeHead(GC)finalize_buffered_end(end) andjs_node_http_res_write_with_cb(write) held the body chunk unrooted acrossimplicit_header. When a program replacesres.writeHead(Next's compression / on-headers), that call runs JS, which can trigger a collection. The collection moved a young chunk, and the stale pointer was read afterwards. The client gotContent-Length: 5130and an empty or truncated body.In Next this lost exactly one of the first 16 concurrent requests on every fresh server. It disappears once the cached page is promoted (around request 17). Plain
node:httphits it too. The chunk is now rooted across the call at both sites.Tests
test-files/test_gap_http_status_message_default.ts: fixed == node; main differs.test-files/test_gap_gc_http_body_chunk_across_write_head.ts: fixed == node; main sends bodies of 0, 2 or 7 bytes instead of 6290 and 7090.Verification (qb9; Next checks on this branch + #13720 merged for
/list)writeHead(200, "")/statusMessage = ""/statusCode = 404; end()/, 10 starts/api/hellostatus lineHTTP/1.1 200 OK== node/852 req/s, 0 errors;/api/hello727 req/s, 0 errorschunked_trailers_are_buffered_and_delivered_together); twoclient_turnloop::native_clienttests abort on main tooNot audited here: other JS values held across JS-running native calls elsewhere in ext-http.