ServerResponse.writeHead isn't setting statusCode, so it always stays at 200

[trentm]

It is setting this._headers such that the actual response status is correct, so this is just about "statusCode" being correct on the ServerResponse object for the rest of the request handler.

On impact of this is on express middleware: e.g. the logger will log "200" for an actual returned status other than 200.

[trentm]

This is for the v0.4 branch. If this seems reasonable, I can do a pull request for master as well. (Or would you prefer just one for master and then cherry-pick to v0.4?)

[felixge]

+1

• Patch LGTM
• I think we should support this
• CLA is signed
• Would like to see this in 0.4 as well, but it is adding a little new behavior

(Haven't run test test yet, but would do before merging, looks good so)

[bnoordhuis]

Thanks, landed in a8f96d3 (master) and bbf7e8e (v0.4).

@felixge re: new behaviour: I consider it a bug fix, not an API change. If it turns out to be a problem for people, we'll revert it.

[koichik]

In the API docs, statusCode is:

When using implicit headers (not calling response.writeHead() explicitly), this property controls the status code that will be send to the client when the headers get flushed.

It is difficult to decide a bug or an API change...
However, I think that we have to add this behavior to the docs.

[felixge]

I'm happy with calling it a bug in order to get it in 0.4, so fine with me :). Thanks for merging ben.

[bnoordhuis]

However, I think that we have to add this behavior to the docs.

/me looks at koichik hopefully

[tj]

IMO writeHead should be removed completely. Extending node is a terrible process right now, you have writeHead with an object, writeHead with an array, the progressive api, any combination, none of which is unified to utilize each other, it's all just ad hoc stuff. In the upcoming Connect 2.x I've more or less dropped support for writeHead

[mikeal]

here are my thoughts:

writeHead should become a shorthand for

statusCode =
for ( i in headers) setHeader(i, headers[i])

We MUST keep support for an array of headers because it's the only way to return multiple headers of the same name. The current implementation is not good, it's inline in writeHead and kind of short circuits the rest of the API in node.

We should do something like req.headers = [] or even req._headers = [] to support this instead of doing it in writeHead.

There is a semantic change here, that comes with a performance penalty:

We would end up deferring on sending the headers until ensureHeadersSent which would mean either the first write on the entity-body or end() if there is no entity-body. Sending them earlier means the requests begin getting parsed earlier.

• If you were streaming a file to the response you would end up not sending the headers until the file handler gets opened and a write comes out which on a network file system or a filesystem under heavy load might be a while.
• If the client is slow and is going to immediately tell us to back off this means that we'll have slightly more writes that have come out of the readStream in to memory. Basically it's ((fs read chunk speed in ms) * client latency in ms) - (fs fd open time + fs read chunk speed for first read chunk).

I think that if writeHead no longer means (write the status and headers NOW!) we should add a method that explicitly sends them so that we can give server authors a way around this performance penalty.

I'm not concerned with Connect's use case. It could be handled by simply wrapping the request and response objects it sends to middleware to defer or change what writeHead actually does. I'm more concerned with the amount of hacks we have in writeHead.

[tj]

In my point of view writeHead has become pretty much useless, it's fine if it becomes an internal or immediate write like you mention, but I see no reason to use it and it's very anti-framework. My point as far as wrapping the methods goes, is that you need to do it in several places right now, and then account for all the wonky ways we can set header fields, it's fine if Node's core wants to ignore the fact that it needs to be extensible, but I won't personally support it.

[mikeal]

Reality check. Most people use writeHead(). Mostly because it's been there longer.

It doesn't fit the pattern for your framework. To say that it's "anti-framework" is a bit much.

Your framework can be responsible for calling writeHead, or not, and you can keep the raw call away from users of your API. That's normal. That's how it should be.

Core should try to provide the best, fastest, and versatile functionality to consumers of it's API. It should not try to optimize it's API for what your framework expects, that's your job. Core should provide the functionality you need, and others need, and you get to pick and choose what you'll use behind your framework.

[tj]

You dont get it, perhaps because you haven't tried to build a higher level framework on node.

[isaacs]

The most "pro-framework" and node-style approach here would be for HTTP headers to be a 2d array, always and everywhere. It's the data structure that maps to the actual protocol, and while the "headers as object" approach is a leaky abstraction that runs into problems in many use cases that we want to support (proxies, cookies, etc.) Unfortunately, dealing with a list of tuples is a huge pain in the ass for many other use cases we want to support (easily managing HTTP conversations in JavaScript.)

The current implementation is somewhat terrible for everyone. We should move this conversation to the dev list, and be prepared to make breaking changes in either 0.5 or 0.7 to fix it.

[mikeal]

You don't get it, because you only live in your own abstraction.

[tj]

You're also ignorant to the fact that nearly everyone using Node for web related work is using either Connect or Express. Being extensible doesn't mean slow, it just means being smart.

[mikeal]

I'd like to build on isaacs approach. But I think we should keep it here until after 0.6 ships because this is definitely something we don't want to do until 0.7.

ServerResponse & ClientRequest (for a few properties)

.statusCode // integer
.headers // 2d array
.setHeader(key, value)
.setHeaders(object || array)
.sendHeaders() // sends StatusCode and headers
.writeHead(status, headers) // reverse compat, sets StatusCode, calls setHeaders and sendHeaders().

[tj]

@mikeal that's the solution I'm looking for, just something unified in the internals so I at least have the chance to proxy and inject functionality.

[mikeal]

i kinda lost it there, i apologize.

[mikeal]

thinking about this a little more, is any of that new API a breaking change?

it is if we change ClientRequest.headers to be consistent but I think that's it so long as writeHead() calls sendHeaders().

if we can limit breaking changes we might be able to think about landing this in 0.5.x.

[tj]

nope, shouldn't be any breakage. It's just tough with things as-is to inject functionality, I'm totally fine with everything opinionated living in Connect/Express-land, it's just really difficult without the internals coming to a single point to tap into. Like you said there would be a very minimal performance change but it would lead to a cleaner core as well

[mikeal]

if we make writeHead call sendHeaders() there is almost zero API change or performance impact.

but you would never need to call writeHead() it for any reason, you could even delete it from the response object entirely if you like, it's really just a shim, which is what it should be.

[mikeal]

sent to the list https://groups.google.com/group/nodejs-dev/browse_thread/thread/f9ddf97342f3d5ff

i'm sorry for losing it like that TJ, i'm removing that comment cause it was totally un-called for.

[tj]

no worries man

[brianc]

Good to see discussion like this. I <3 the node community.