Repository navigation
Duplicated response.writeHead calling is not checked in specific case #20932
Description
- 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).
Line 471 in 9a02de7
In
ServerResponse.prototype.writeHead, the OutgoingMessage.prototype.setHeader is called only when current instance has header data (i.e. it has this[outHeadersKey]).Line 224 in 9a02de7
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.
Line 234 in 9a02de7
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.
Line 234 in 9a02de7
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.
Activity
1ns3cur3 commentedon May 24, 2018
I also found that if you use different
statusCode(say 204) in first request and you areres.end('ok')later; that doesn't setTransfer-Encoding: chunkedin response and res.body is not sent.Though this doesn't work with
404or302.Either only
204screws res up or different classes of status code don't work with it.gireeshpunathil commentedon Jun 25, 2018
Full context available in #21289 and the referred link. In short:
Probably not, but given that there is an optimization to bypass header caching in the fast path, and given that calling
writeHeadtwice is inhibited in the documentation, I would say this is fine.anseki commentedon Jun 25, 2018
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
writeHeadis called twice.Then, document should warn about that.
Also, the warning may help us. For example, node-static calls the
writeHeadtwice. 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 commentedon Sep 25, 2018
this has been documented with explanation to the behavior. Closing.
getify commentedon Sep 25, 2018
@gireeshpunathil can you link me to the doc change itself, or at least an updated version of the documentation?
gireeshpunathil commentedon Dec 30, 2019
@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 commentedon Dec 30, 2019
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 commentedon Dec 30, 2019
@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?