Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Closed
wants to merge 1 commit into from

8 participants

@trentm
Owner

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
Owner

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

@koichik koichik referenced this pull request from a commit
@koichik koichik Doc improvements
corresponds to #1374 and #1334.
691497b
@tj
tj commented

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
tj commented

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
tj commented

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
tj commented

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
tj commented

@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
tj commented

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
tj commented

no worries man

@brianc

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

@koichik koichik referenced this pull request from a commit in koichik/node
@trentm trentm http: fix setting ServerResponse.statusCode in writeHead
Fixes #1374.
bbf7e8e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 21, 2011
  1. @trentm
This page is out of date. Refresh to see the latest.
Showing with 67 additions and 0 deletions.
  1. +1 −0  lib/http.js
  2. +66 −0 test/simple/test-http-status-code.js
View
1  lib/http.js
@@ -805,6 +805,7 @@ ServerResponse.prototype.writeHead = function(statusCode) {
reasonPhrase = STATUS_CODES[statusCode] || 'unknown';
headerIndex = 1;
}
+ this.statusCode = statusCode;
var obj = arguments[headerIndex];
View
66 test/simple/test-http-status-code.js
@@ -0,0 +1,66 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var http = require('http');
+
+// Simple test of Node's HTTP ServerResponse.statusCode
+// ServerResponse.prototype.statusCode
+
+var testsComplete = 0;
+var tests = [200, 202, 300, 404, 500];
+var testIdx = 0;
+
+var s = http.createServer(function(req, res) {
+ var t = tests[testIdx];
+ res.writeHead(t, {'Content-Type': 'text/plain'});
+ console.log('--\nserver: statusCode after writeHead: '+res.statusCode);
+ assert.equal(res.statusCode, t);
+ res.end('hello world\n');
+});
+
+s.listen(common.PORT, nextTest);
+
+
+function nextTest () {
+ if (testIdx + 1 === tests.length) {
+ return s.close();
+ }
+ var test = tests[testIdx];
+
+ http.get({ port: common.PORT }, function(response) {
+ console.log('client: expected status: ' + test);
+ console.log('client: statusCode: ' + response.statusCode);
+ assert.equal(response.statusCode, test);
+ response.on('end', function() {
+ testsComplete++;
+ testIdx += 1;
+ nextTest();
+ });
+ });
+}
+
+
+process.on('exit', function() {
+ assert.equal(4, testsComplete);
+});
+
Something went wrong with that request. Please try again.