Allowing HEAD method to work with keep-alive

[josephhackman]

Fixes: #28438

This approach specifically retains the non-hanging of 204 requests while allowing HEAD to function correctly with keeping sockets open. I check for 204 + 304 specifically rather than the HEAD method because the method is in _http_server, not _http_outgoing, though it could be passed along if that's preferred.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[bnoordhuis]

Mostly LGTM, thanks for doing this. Can you also test the 304 status code?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/32247/

[josephhackman]

It seems that the only tests failing are flakey. Is there additional action required on my part?

[bnoordhuis]

@josephhackman No action on your part is needed but since this is a fairly subtle change it'd be good to get at least one more sign-off from a @nodejs/http member.

[ronag]

Just as a side note. HEAD requests have an edge case due to which many HTTP clients (including the node one) will always close the connection and ignore keep-alive.

#12396
nodejs/undici#258

[josephhackman]

Just as a side note. HEAD requests have an edge case due to which many HTTP clients (including the node one) will always close the connection and ignore keep-alive.

#12396
mcollina/undici#258

This isn't quite true. The node client will close a the connection on a HEAD response if neither the Content-Length or Content-Encoding headers are set. This PR contains a test that empirically verifies this, using the node server and client. This PR does not make any changes to the node client, so it will continue doing what it did before, which is sometimes closing the socket on head-requests.

These headers are allowed by the HTTP standard, so this change should keep the node server just as compliant as it always was, while allowing any properly-implemented client to be substantially faster while sending many HEAD requests to collect metadata.

On the other hand, it is possible that non-standards-compliant clients exist that would malfunction seeing these headers on a HEAD request. I think that's a minimal risk because most people don't implement their own HTTP client library, but many people do mess with the headers on their servers. This leads me to expect that clients tend to be standards-compliant, and if anything, hardened.

Refs: https://tools.ietf.org/html/rfc7231#section-3.3
Refs: https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/34434/

[ronag]

This needs another approval. I'm not comfortable in my own knowledge here to give the second one.

@nodejs/http

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/35102/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/35224/

[aduh95]

Landed in 7afa533 🎉 Thanks a lot for your contribution!

[fluggo]

I'm not convinced this was a good change. My HEAD methods with status 200 now send content-length: 0 by default, and this is very much not allowed by RFC7230:

A server MAY send a Content-Length header field in a response to a HEAD request (Section 4.3.2 of [RFC7231]); a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a response if the same request had used the GET method. 

Seems to me the sensible default would have been transfer-encoding: chunked.