Potential breaking change on v14.17.0

[mmarchini]

• Version: v14.17.0
• Platform: OS X
• Subsystem: http

What steps will reproduce the bug?

Run the snippet below (from How do I create a HTTP server):

const http = require('http');

const requestListener = function (req, res) {
  res.writeHead(200);
  res.end('Hello, World!');
}

const server = http.createServer(requestListener);
server.listen(8080);

And call the server with the following command:

curl -i localhost:8080 -X HEAD

Running the server on v14.16.1, the curl command will return 0 with the following output:

$ curl -i localhost:8080 -X HEAD
Warning: Setting custom HTTP method to HEAD with -X/--request may not work the
Warning: way you want. Consider using -I/--head instead.
HTTP/1.1 200 OK
Date: Fri, 04 Jun 2021 04:15:27 GMT
Connection: keep-alive
Keep-Alive: timeout=5

Running the server on v14.17.0, the curl command will exit with code 18 and the following error:

$ curl -i localhost:8080 -X HEAD
Warning: Setting custom HTTP method to HEAD with -X/--request may not work the
Warning: way you want. Consider using -I/--head instead.
HTTP/1.1 200 OK
Date: Fri, 04 Jun 2021 04:16:58 GMT
Connection: keep-alive
Keep-Alive: timeout=5
Transfer-Encoding: chunked

curl: (18) transfer closed with outstanding read data remaining

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

Within a major, no header that could potentially destructively affects HTTP clients behavior should be introduced.

What do you see instead?

Transfer-Encoding: chunked is introduced, causing curl to exit with an error code.

Additional information

I noticed this error on this Restify test recently. I'm not entirely sure if this should be considered a breaking change, but it seems like one. I couldn't determine which commit introduced it yet though.

cc @nodejs/http (and @nodejs/tsc @nodejs/lts for visibility)

[mcollina]

My gut is on #34231. I think we might want to do a revert, or a more fundamental fix of the problem.

[mcollina]

@nodejs/release @nodejs/tsc wdyt?

[ronag]

We don't allow HEAD + keepAlive in undici due to the various edge cases involved. I'm fine with reverting #34231.

[mmarchini]

Since this is breaking, I think we should revert on v14 unless someone has an immediate fix in mind.

[joyeecheung]

+1 to revert on v14

[targos]

Only on v14?

[mmarchini]

reverting it on v16 would be a breaking change, wouldn't it?

[mcollina]

It's not just breaking, it's not correct. I do not think there is a safe and spec-compliant way to implement this on top of our current API.

[mhdawson]

+1 to revert on 14.x

In terms of reverting on 16.x I see a few things in favor

• @mcollina indicates it's just not correct
• It was not marked as SemVer major when landed (not even SemVer patch)

On the other side I guess it would be breaking for the reporter of #28438

[mmarchini]

It was not marked as SemVer major when landed (not even SemVer patch)

Is that something we do per our policy? Or is it something we evaluate on a case-by-case basis?

[targos]

PR to revert: #38949