Fix 'connection' header, copying of headers, and support Windows named pipes #294

Closed
wants to merge 2 commits into
from

4 participants

@gilad61

This pull request is to replace #260 (#260)

It contains the same changes + TESTS:
1. Set the connection header only if not already set by the target server.
2. Copy response headers case-insensitively

In addition it contains another fix:
support unix domain sockets and windows named pipes (socketPath) - This was transparent in node 0.6.x by using port for named pipe, but in node 0.8.x it should be passed explicitly.

yosefd and others added some commits Jul 29, 2012
@yosefd yosefd - support unix donain sockets and windows named pipes (socketPath) on…
… node 0.8.x. On node 0.6.x the support was opaque via port, but on the new node, socketPath should be set explicitely.

- avoid adding multiple headers with the same name
- fix bug in setting connection
3438e54
@gilad61 gilad61 Add tests for headers bug fixes 876157f
@mmalecki

Doing a copy of headers on every request isn't a good thing to do - I'm worried about memory usage. Can you explain exactly why is it needed (an example would be appreciated)? Test case doesn't tell me much.

@gilad61

If response headers are copied using the res.writeHead() function, then the copy is case sensitive. It means that eventually we may have two headers with same name but different casing, for example: x-My-Header: "a", x-my-header: "b", and then it is unpredictable which value will be taken by the client, and doesn't meet HTTP standard.
HTTP headers are case insensitive, so the copy needs to be done with res.setHeader(), which overrides existing headers case-insensitively.

I don't see a difference in mem usage as in one case we copy all the headers at once, and in another case we copy them one by one.
I would even say the opposite - if you use the current implementation you'll use more memory.
For example:
If I have "myheader: x", and then I get a request with "MYHEADER: y", then I'll end up with two headers. And same thing if I get another header "MyHeader: z", and "MyHeAdEr: a" and so on...
But - if I copy them with setHeader() then the headers will be overridden and I will have only one header with the same name.

@indexzero
nodejitsu member

@gilad61 @mmalecki This is a subtle but good fix; we should be setting header names consistently. res._renderHeaders() is still case sensitive even in the latest stable versions of node:

@indexzero
nodejitsu member

I spoke with @isaacs, this behavior is not going to change even in 0.10.0, but likely in 0.12.0. So going to pull this in if all the tests pass.

@indexzero
nodejitsu member

Cherry-picked. Thanks

@indexzero indexzero closed this Mar 9, 2013
@indexzero indexzero referenced this pull request Mar 9, 2013
Closed

Unix socket support #104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment