lib: http server multi same-named headers support

[PeterDaveHello]

This is my first PR to nodejs, hope didn't do anything stupid or funny ... Thanks.

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

lib/_http_outgoing.js

Description of change

Let http server support response multiple same-named header as RFC2612
defined, this feature is also used in http/2.0 "server push" feature.

Fix #3591

[PeterDaveHello]

On FreeBSD, I got something like this on the branch for this PR by running test:

STATUS: 200                                                                                                                    [9/1867]
HEADERS: 
{ 'x-foo': 'keyboard cat',
  'x-bar': 'baz',
  date: 'Wed, 18 May 2016 20:53:39 GMT',
  connection: 'close',
  'transfer-encoding': 'chunked' }
assert.js:90
  throw new assert.AssertionError({
  ^
AssertionError: 'keyboard cat' == 'bar'
    at ClientRequest.<anonymous> (/usr/home/peter/node/test/parallel/test-http-mutable-headers.js:106:16)
    at ClientRequest.g (events.js:286:16)
    at emitOne (events.js:96:13)
    at ClientRequest.emit (events.js:188:7)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:469:21)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:105:23)
    at Socket.socketOnData (_http_client.js:359:20)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:172:18)
Command: out/Release/node /usr/home/peter/node/test/parallel/test-http-mutable-headers.js
=== release test-http-write-head ===                                           
Path: parallel/test-http-write-head
assert.js:90
  throw new assert.AssertionError({
  ^
AssertionError: '1, 2' == '2'
    at IncomingMessage.<anonymous> (/usr/home/peter/node/test/parallel/test-http-write-head.js:42:14)
    at emitNone (events.js:91:20)
    at IncomingMessage.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:926:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)
Command: out/Release/node /usr/home/peter/node/test/parallel/test-http-write-head.js
[01:18|% 100|+ 1111|-   2]: Done                                               
Makefile:118: recipe for target 'test' failed
gmake: *** [test] Error 1

[addaleax]

These test failures reflect the expectation that when sending multiple, identically named HTTP headers, the last one takes precedence. They would need to be changed in this PR, and, ideally, new ones would be added that test the new behaviour.

/cc @nodejs/http

[PeterDaveHello]

Thank you @addaleax !

[PeterDaveHello]

I updated the test and now it passed, but I'm not sure if I did it right, waiting for review now, I'm new to this, please feel free to correct me, many thanks.

[addaleax]

shouldn’t the output here be keyboard cat, bar? For incoming http messages node glues the returned header values together with ', '.

[PeterDaveHello]

hmmmm ... I'll take a look at writeHead function, thanks.

[jasnell]

I'm not quite understand what this PR is looking to achieve. The http implementation already support multiple same-named headers. There are a specific subset of headers that may only be sent once (like Content-Length) but for the others it's certainly already possible to send multiple headers with the same name.

[addaleax]

Uh, yeah, @jasnell’s right. I think that original issue linked above can be closed then?

[dougwilson]

For outgoing responses, I've never had an issue sending multiple same-named headers as different header lines in the underlying response. Express.js even has an API res.append to easily add header lines one-by-one.

[PeterDaveHello]

@jasnell @dougwilson sorry that since that issue #3591 has no any response for a while, so I thought it's still a problem, I should be more careful, that's my fault. Actually, @dougwilson you are right, res.append in expressjs looks great, so I'm wondering if we could support using response.setHeader to set the same-named headers? Currently not, and this PR will make it work, just not sure if this will be acceptted or not. Thank you guys for your comments!

[dougwilson]

This PR does seem to break additional functionality, for example, OutgoingMessage.prototype.getHeader will not work correctly after the change, because it will look up in _headers using _headers[name.toLowerCase()] every time, not taking into account any headers after the first added one.

It also seems to have the edge case of the following not working:

res.setHeader('X-Log', 'test')
res.setHeader('X-Log', 'test2')
res.setHeader('X-Log0', 'test3')

assert.equal(res.getHeader('X-Log0'), 'test3') // fails with this PR

Though of course the PR is currently very rough, as it doesn't look like the current accesses to _headers are fully worked out to take into account the new key patterns. I'm not sure if this is a good addition to the setHeader API, especially since it already has the functionality you are looking for, just the API is slightly different from what you are expecting. User-land modules can easily make a different API for it (like Express.js has done), and the word set to me implies that it's setting that header name to be that value, not adding a new header line, keeping previous lines. That's my 2 cents.

[PeterDaveHello]

@dougwilson thanks for your review, if this function/feature can be accepted, I'll like to fix the problems you mentioned 😄

[jasnell]

While I definitely appreciate the contribution, I certainly doubt that this PR would land.

[PeterDaveHello]

@jasnell it's okay if it won't be accepted, no mind.

[jasnell]

Given the responses, closing this. Thank you for the contribution tho!