Duplicated response.writeHead calling is not checked in specific case

[anseki]

• Version: v8.11.1
• Platform: Win 64-bit
• Subsystem:

As it is documented, response.writeHead method should be called only once.
When the response.writeHead method is called twice, an error ("Can't set headers after they are sent.") is thrown if response.setHeader method was called. And the error is not thrown if the response.setHeader method was not called.

For example:

Run this code as a server:

'use strict';

require('http').createServer((request, response) => {
  request.addListener('end', () => {

    console.log('Call: writeHead #1');
    response.writeHead(200, {
      'Content-Type': 'text/plain',
      'X-COUNT': 1
    });

    console.log('Call: writeHead #2');
    response.writeHead(200, {
      'Content-Type': 'text/plain',
      'X-COUNT': 2
    });

    response.end('Hello, World!');

  }).resume();
}).listen(8080);

And run this code as a client (using request module):

'use strict';

require('request')({
  url: 'http://localhost:8080/'
}, (error, response, body) => {
  console.log('error: ', error);
  console.log('statusCode: ', response && response.statusCode);
  console.log('headers:');
  console.log(response && response.headers);
  console.log('body: ', body);
});

That works fine without an error but the code as server calls the response.writeHead method twice.

So, run this code as a server instead:

'use strict';

require('http').createServer((request, response) => {
  request.addListener('end', () => {

    console.log('Call: setHeader');
    response.setHeader('foo', 'bar');

    console.log('Call: writeHead #1');
    response.writeHead(200, {
      'Content-Type': 'text/plain',
      'X-COUNT': 1
    });

    console.log('Call: writeHead #2');
    response.writeHead(200, {
      'Content-Type': 'text/plain',
      'X-COUNT': 2
    });

    response.end('Hello, World!');

  }).resume();
}).listen(8080);

And run the client above.

Then, an error "Can't set headers after they are sent." is thrown.
Just only the response.setHeader method was added to the second server.

Is this correct behavior?
Of course the response.writeHead method should not be called again. However I feel strange about that it is checked only when the response.setHeader method was called.
It seems that this issue is relevant to #8446, maybe. I take notice of an error that is thrown by the response.setHeader method.

In OutgoingMessage.prototype.setHeader, the error is thrown if the headers are already sent (rendered).

node/lib/_http_outgoing.js

Line 471 in 9a02de7

throw new ERR_HTTP_HEADERS_SENT('set');

In ServerResponse.prototype.writeHead, the OutgoingMessage.prototype.setHeader is called only when current instance has header data (i.e. it has this[outHeadersKey]).

node/lib/_http_server.js

Line 224 in 9a02de7

if (this[outHeadersKey]) {

In other words, the response.setHeader method that checks duplicated calling is called only when that method was already called (i.e. this[outHeadersKey] was already set).
Also, duplicated calling is checked after headers were merged.

node/lib/_http_server.js

Line 234 in 9a02de7

if (k === undefined && this._header) {

I think that the checking should be moved to after if ... else.

    // only progressive api is used
    headers = this[outHeadersKey];
  } else {
    // only writeHead() called
    headers = obj;
  }
  if (this._header) {
    throw new ERR_HTTP_HEADERS_SENT('render');
  }

That is, duplicated calling should be checked whether headers were merged or not.

Or, checking at top of writeHead is better because it has to do nothing when it throws an error.

function writeHead(statusCode, reason, obj) {
  if (this._header) {
    throw new ERR_HTTP_HEADERS_SENT('render');
  }

  var originalStatusCode = statusCode;

And also, it seems that there is another problem in that checking.
Run this code as a server instead:

'use strict';

require('http').createServer((request, response) => {
  request.addListener('end', () => {

    console.log('Call: setHeader');
    response.setHeader('foo', 'bar');

    console.log('Call: writeHead #1');
    response.writeHead(200, {
      'Content-Type': 'text/plain',
      'X-COUNT': 1
    });

    console.log('Call: writeHead #2');
    response.writeHead(200, {
      '': 'head'
    });

    response.end('Hello, World!');

  }).resume();
}).listen(8080);

This is the same as the code that throws an error except for last header name. However this does not throw an error.

After header were merged, duplicated calling is checked only when the last header name is undefined.

node/lib/_http_server.js

Line 234 in 9a02de7

if (k === undefined && this._header) {

A value of the k was updated repeatedly, and it has last header name or initial value. And duplicated calling is checked by the response.setHeader method in a loop only when the k is truthy.
If the k === undefined means that the response.setHeader method never be called, that was not sufficient.
I think that checking the k is not needed (i.e. if (this._header) {) because duplicated calling can be checked even if that was already checked in the response.setHeader method.
This problem also is solved by changing above, as I indicated.

[1ns3cur3]

I also found that if you use different statusCode (say 204) in first request and you are res.end('ok') later; that doesn't set Transfer-Encoding: chunked in response and res.body is not sent.

Though this doesn't work with 404 or 302.

Either only 204 screws res up or different classes of status code don't work with it.

[gireeshpunathil]

Full context available in #21289 and the referred link. In short:

Is this correct behavior?

Probably not, but given that there is an optimization to bypass header caching in the fast path, and given that calling writeHead twice is inhibited in the documentation, I would say this is fine.

[anseki]

I see.
I think, we confuse because there are two cases: a case that an error is thrown and a case that an error is not thrown when the writeHead is called twice.
Then, document should warn about that.
Also, the warning may help us. For example, node-static calls the writeHead twice. Of course that is a bug of node-static, it is not a bug of Node.js. However, I had to read the source code of Node.js to find the bug of node-static because I didn't know why an error is not thrown, in almost cases.

[gireeshpunathil]

this has been documented with explanation to the behavior. Closing.

[getify]

@gireeshpunathil can you link me to the doc change itself, or at least an updated version of the documentation?

[gireeshpunathil]

@getify - sorry, I missed this question. the behavior difference between writeHead and setHeader w.r.t repeated header updates are documented here:

https://nodejs.org/dist/latest-v13.x/docs/api/http.html#http_response_setheader_name_value

https://nodejs.org/dist/latest-v13.x/docs/api/http.html#http_response_writehead_statuscode_statusmessage_headers

hope this helps

[getify]

That documentation does not, IMO, spell out an explanation or warning about the case listed here. I've read it several times and I still find it lacking in clarity.

Knowing this problem (from this issue), I can sorta infer that the bypass-caching-headers fast path optimization is the key to understanding this gotcha... but only barely. If I didn't know about this issue, it is not clear to me from reading the docs that you will get an error (on double call) only if you previously called setHeader. I still find that bizarre and the docs do not warn me about it.

When docs say "don't call this twice", a reasonable person expects that violating that will give an error. If there are optimizations in place that prevent an error from being displayed, such that the second call silently fails, this is a gotcha. And the docs do not warn about that gotcha.

[gireeshpunathil]

@getify - thanks. I too read it many times now; and I admit something can be improved there, but not able to figure out how (I wrote those new sentences earlier) . Do you want to try phrasing something?