Repository navigation
Potential breaking change on v14.17.0 #38922
Description
- 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=5Running 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 remainingHow 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)
Activity
mcollina commentedon Jun 4, 2021
My gut is on #34231. I think we might want to do a revert, or a more fundamental fix of the problem.
mcollina commentedon Jun 4, 2021
@nodejs/release @nodejs/tsc wdyt?
ronag commentedon Jun 4, 2021
We don't allow HEAD + keepAlive in undici due to the various edge cases involved. I'm fine with reverting #34231.
mmarchini commentedon Jun 4, 2021
Since this is breaking, I think we should revert on v14 unless someone has an immediate fix in mind.
joyeecheung commentedon Jun 4, 2021
+1 to revert on v14
targos commentedon Jun 4, 2021
Only on v14?
mmarchini commentedon Jun 4, 2021
reverting it on v16 would be a breaking change, wouldn't it?
mcollina commentedon Jun 4, 2021
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 commentedon Jun 4, 2021
+1 to revert on 14.x
In terms of reverting on 16.x I see a few things in favor
On the other side I guess it would be breaking for the reporter of #28438
mmarchini commentedon Jun 4, 2021
Is that something we do per our policy? Or is it something we evaluate on a case-by-case basis?
Revert "http: make HEAD method to work with keep-alive"
targos commentedon Jun 6, 2021
PR to revert: #38949
Revert "http: make HEAD method to work with keep-alive"
Revert "http: make HEAD method to work with keep-alive"
Revert "http: make HEAD method to work with keep-alive"