fix(node/http): disable chunked request if Content-Length header is specified

[PolarETech]

Fixes #2754

This PR disables applying chunked encoding to the request body if the following conditions are met.

• Transfer-Encoding: chunked is not specified in the header options
• Content-Length is specified in the header options

References

https://nodejs.org/api/http.html#httprequesturl-options-callback
https://datatracker.ietf.org/doc/html/rfc7230#section-3.3

Tests

No applicable test for this issue has been found in the Node test suite. Unit tests have been added.

[cjihrig]

I ported this test to Node. Unless I messed up, the test didn't work in Node (server sent back a 400). Have you ran these tests in Node?

[PolarETech]

I am aware of this.
I think it is difficult to make the behavior completely the same as Node because of the spec of Deno's fetch() which is used inside node/http.request().

For Node's http.request():
If both Content-Length and Transfer-Encoding headers are specified, both Content-Length and Transfer-Encoding headers are sent. The request body is chunked encoding.

For Deno's fetch():
Even if both Content-Length and Transfer-Encoding headers are specified¹, if the body is a ReadableStream, only the Transfer-Encoding header is sent.

This unit test is designed to make sure that the Content-Length header is not sent depending on the current behavior of Deno's fetch(), so it is not passed by Node.

For reference, the HTTP logs obtained by Wireshark during the test run are attached below.

Node:

POST / HTTP/1.1
Content-Type: text/plain; charset=utf-8
Content-Length: 11
Transfer-Encoding: chunked
Host: localhost:56750
Connection: close

6
hello 
5
world
0

HTTP/1.1 400 Bad Request
Connection: close

// When the Node's HTTP server receives both "Content-Length" and "Transfer-Encoding" headers,
// it fires the "clientError" event and returns HTTP Status Code 400.

Deno:

POST / HTTP/1.1
content-type: text/plain; charset=utf-8
transfer-encoding: chunked
accept: */*
accept-language: *
user-agent: Deno/1.26.1
accept-encoding: gzip, br
host: localhost:56390

6
hello 
5
world
0

HTTP/1.1 200 OK
Date: Fri, 07 Oct 2022 19:28:48 GMT
Content-Type: text/plain;charset=UTF-8
Content-Length: 2

ok

Footnotes

1. The current implementation does not pass the headers option to fetch() in the relevant code of node/http.ts, but the same result will be obtained if it is changed to do so.
   https://github.com/denoland/deno_std/pull/2755/files#diff-07717ecd86915a087f8759f720b0217ab7c3c9f473aa8f4772aeae5dbb638a92R127-R131
   I would like to create a separate PR for not passing the headers option to fetch(). ↩

[kt3k]

Is this 2nd test case necessary for checking the fix?

[PolarETech]

The 2nd test case has been added to increase test coverage for the condition, but it is certainly not necessary to confirm the fix of the issue.

[kt3k]

Ok, I confirmed that Node.js sends transfer-encoding: chunked when both content-length: N and transfer-encoding: chunked are specified. This test case seems useful to me. But how about rewriting the server part of the test case with serve function of std/http/server.ts? Because this test case depends on non-compliant behavior of http.Server and server part is not the subject of the test case, I think it makes more sense to use std/http here.

[PolarETech]

Thanks for your advice. I have changed the unit test cases. (d38c018)

[cjihrig]

Left another comment.

To be honest, I'm not sure if this should be landed or not. Your reasoning seems legit, but the fact that there are no Node tests and the change behaves differently from Node based on my testing makes me think we might need some other changes first.

@kt3k thoughts?

[cjihrig]

Suggested change

	!Number.isNaN(parseInt(headers["content-length"]))
	!Number.isNaN(Number.parseInt(headers["content-length"], 10))

[kt3k]

@PolarETech Sorry for the delay in review. I missed your last comment.. 😓

Now this looks good to me. Thanks for your contribution! LGTM