http: include Content-Length in HEAD responses for keep-alive#61919
http: include Content-Length in HEAD responses for keep-alive#61919erdinccurebal wants to merge 1 commit intonodejs:mainfrom
Conversation
When an HTTP server responds to a HEAD request with res.end(), the response does not include a Content-Length header by default. This causes the HTTP parser on the client side to be unable to determine message boundaries, which breaks keep-alive connections for HEAD requests. GET responses already include Content-Length: 0 by default when res.end() is called without data. This change applies the same behavior to HEAD responses (and other bodyless responses) by adding Content-Length when known and not explicitly removed. Fixes: nodejs#28438
|
Review requested:
|
pimterry
left a comment
There was a problem hiding this comment.
Thanks for looking into this @erdinccurebal! Unfortunately, I don't think this is correct approach, sorry.
HEAD responses (spec here) are guaranteed (MUST NOT) to have no body, so there is no message framing ambiguity in the original bug. Problems with keep-alive are a client-side bug, or plausibly some other keep-alive problem on the server side, but the client doesn't need any more information or headers to know when the HEAD response ends (because it ends immediately, always).
On the flip side, I think this will potentially send wrong headers that will break some existing code.
The HEAD response headers are supposed to each exactly match the headers for an equivalent GET response, or be omitted. I.e. if the GET response content-length is non-zero, the HEAD response should be non-zero content-length, or content-length should be omitted entirely. They should only be content-length: 0 if the equivalent GET would also return that same header.
With this change, if there's any existing code that handles HEAD explicitly and skips generating the GET-equivalent body (just calls res.end()) from what I can see it seems we'd now start automatically sending content-length: 0 with that response, which is incorrect and would break things (e.g. anybody using HEAD to query a file download size before actually downloading).
I think we need to look at this on the client-side instead, and investigate parsing & keep-alive behaviour there. We do definitely need to be able to handle receiving no content-length header on HEAD responses because the spec explicitly lists that as something you might normally want to do.
We could still do this anyway, it's arguably nice UX in some cases, but I think it would be only for non-zero scenarios to avoid the bug described above.
Summary
HEAD responses from
http.createServerdo not include aContent-Lengthheader by default whenres.end()is called without explicit headers. This breaks HTTP keep-alive for HEAD requests because the client-side HTTP parser cannot determine message boundaries.GET responses already include
Content-Length: 0by default. This change applies the same behavior to HEAD (and other bodyless) responses.Before
After
Changes
lib/_http_outgoing.js: In_storeHeader(), when the response has no body (!this._hasBody), also emitContent-Lengthheader if the content length is known and was not explicitly removed by the user.test/parallel/test-http-head-response-content-length-keep-alive.js: New test verifying that both GET and HEAD responses includeContent-Length: 0whenres.end()is called.Reproduction
Fixes: #28438