http: process array headers _after_ setting up agent #16568
Conversation
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 |
|
ping @nodejs/http |
|
Even though this isn't semver-major, I've added |
| 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.
|
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 |
BridgeAR
added
the
author ready
|
One more CI before landing https://ci.nodejs.org/job/node-test-pull-request/12622/ |
|
@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. |
|
@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. |
|
Landed in 4404c76 |
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
headersoption as anArray. 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-aliveregardless of how you set your http client up. The same request using an object to set headers setsConnection: close. This can lead to an unexpected client "hang" as the connection remains open till timeout. Comparehttp.request({ port: 1337, path: '/', headers: { foo: 'bar' }})withhttp.request({ port: 1337, path: '/', headers: [ 'foo', 'bar' ]}). You getclosewith the former andkeep-alivewith the latter.This PR rearranges the logic to set the array headers later, allowing the keep-alive logic to run before actually setting the
Connectionheader.The other two problems with array headers is that they you don't get the
Hostheader set andoptions.authwon'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.