http: process array headers _after_ setting up agent - #16568
Conversation
Sorry, something went wrong.
Added tests to clarify the implicit behaviour of array header setting vs object header setting
|
https://ci.nodejs.org/job/node-test-commit/13535/ all good except for some git failures on windows |
Sorry, something went wrong.
|
ping @nodejs/http |
Sorry, something went wrong.
|
Even though this isn't semver-major, I've added dont-land-on for all of the LTS branches due to the potential for breakage involved in touching this stuff. This could be revisited I guess if someone comes complaining about this broken functionality in an LTS version. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
| expectHeaders.host = `localhost:${this.address().port}`; | ||
| } | ||
|
|
||
| // no Authorization header when you set headers an array |
There was a problem hiding this comment.
as array?
And it would be nice to use caps for the first character of each comment.
Both applies to the comment above as well.
Sorry, something went wrong.
|
I am not completely sure if I would not rather deprecate the functionality. Fixing it is probably a good idea nevertheless. Due to being indecisive I will not review this but I would like to get the opinion of other @nodejs/collaborators |
Sorry, something went wrong.
|
One more CI before landing https://ci.nodejs.org/job/node-test-pull-request/12622/ |
Sorry, something went wrong.
Sorry, something went wrong.
|
@BridgeAR I proposed removing it ages ago, but at the time couldn't get much agreement I remember :) I'm still in favor of removing any and all undocumented features, including this one. |
Sorry, something went wrong.
|
@ronkorving I am +1 for deprecating this functionality. Even after this PR landed, it is still going to be undocumented. So if you have the time, please feel free to open a PR to add a deprecation for it. That way we can also likely remove it after some time. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
LGTM. I would prefer for the headers array support to be removed.
Sorry, something went wrong.
Added tests to clarify the implicit behaviour of array header setting vs object header setting PR-URL: nodejs#16568 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Added tests to clarify the implicit behaviour of array header setting vs object header setting PR-URL: #16568 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Added tests to clarify the implicit behaviour of array header setting vs object header setting PR-URL: #16568 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Added tests to clarify the implicit behaviour of array header setting vs object header setting PR-URL: #16568 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Added tests to clarify the implicit behaviour of array header setting vs object header setting PR-URL: nodejs#16568 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
http client accepts the headers option as an Array. This is not documented but is used in 4 places in our tests: parallel/test-http, parallel/test-http-server-multiheaders, parallel/test-http-server-multiheaders2, parallel/test-http-upgrade-client. I believe it traces all the way back to an old API that was refactored and this support was left in. There is some utility in it, however: it's likely to be slightly more efficient as it skips a few tests and does header setting in batch and sends them straight away, it's also the only way to set full multi-headers (see parallel/test-http-server-multiheaders for an example of this as well as the test case in this PR).
There are a few drawbacks, however, one of which I believe is a bug and is fixed with this PR: the logic to determine whether to use a keep-alive connection happens after the array headers are stored and sent, so it ends up defaulting to Connection: keep-alive regardless of how you set your http client up. The same request using an object to set headers sets Connection: close. This can lead to an unexpected client "hang" as the connection remains open till timeout. Compare http.request({ port: 1337, path: '/', headers: { foo: 'bar' }}) with http.request({ port: 1337, path: '/', headers: [ 'foo', 'bar' ]}). You get close with the former and keep-alive with the latter.
This PR rearranges the logic to set the array headers later, allowing the keep-alive logic to run before actually setting the Connection header.
The other two problems with array headers is that they you don't get the Host header set and options.auth won't work. The test case in this PR confirms that. These two are arguably bugs, but it's going to require some non-trivial work to make it happen (particularly if we want to avoid directly appending the submitted array, which I think would be our goal?). So I want to check here first before doing anything about that.
I also have a branch where I'm toying with removing array headers but it's sufficiently embedded in both _http_client.js and _http_outgoing.js and we have 4 test cases showing examples that I'm uneasy just pushing ahead with that as it'd be a breaking change and it's not unlikely that there's lot of people using array headers out there in the wild. I haven't even looked at what would happen on the server side for sending headers since _http_outgoing.js appears to make it possible to do the same thing there.
I think my preference would be to formalise array headers as an option and even document it, but it'd require fixing auth and host. Interested to hear what others think.