Skip to content

http: process array headers _after_ setting up agent

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.

Merge request reports

Loading