http: invoke callback with ERR_STREAM_DESTROYED if the socket is destroyed

[Lxxyx]

Fixes: #36673
Refs: #29227 (comment)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[Lxxyx]

ping @ronag

[ronag]

commit description is wrong, should be invoke callback with ERR_STREAM_DESTROYED

[ronag]

I kind of think this check should live directly in .end() and .write() and not (only) in writeRaw or lot's of other side effects might apply before the check.

[Lxxyx]

I kind of think this check should live directly in .end() and .write() and not (only) in writeRaw or lot's of other side effects might apply before the check.

The destroyed check has been added to both .end() and .write().

There is also a small question: should we move the destroyed check from _writeRaw() to _send(). Before calling _writeRaw(), _send() may modify outputData.

[ronag]

There is also a small question: should we move the destroyed check from _writeRaw() to _send(). Before calling _writeRaw(), _send() may modify outputData.

I'm unsure whether we need the check in _writeRaw at all.

[Lxxyx]

I'm unsure whether we need the check in _writeRaw at all.

I need a little time to improve the code and add unit tests, the situation is a little more complicated than expected

[Lxxyx]

From this PR (#31818), both .end() and .write() have checked the destroyed state and will throw the ERR_STREAM_DESTROYED error.

node/lib/_http_outgoing.js

Lines 725 to 730 in b12127a

	leterr;
	if(msg.finished){
	err=newERR_STREAM_WRITE_AFTER_END();
	}elseif(msg.destroyed){
	err=newERR_STREAM_DESTROYED('write');
	}

We should only need to check req/res.socket.destroyed and pass ERR_STREAM_DESTROYED to the callback. @ronag

[ronag]

I think something along these lines would be good and quite close to what writable.js does:

diff--gita/lib/_http_outgoing.jsb/lib/_http_outgoing.jsindex50ef063241..6e6ab8eb92100644---a/lib/_http_outgoing.js+++b/lib/_http_outgoing.js
@@ -51,10+51,6 @@ const{ Buffer }=require('buffer');constcommon=require('_http_common');constcheckIsHttpToken=common._checkIsHttpToken;constcheckInvalidHeaderChar=common._checkInvalidHeaderChar;-const{-defaultTriggerAsyncIdScope,-symbols: { async_id_symbol }-}=require('internal/async_hooks');const{codes: {ERR_HTTP_HEADERS_SENT,
@@ -689,23+685,6 @@ OutgoingMessage.prototype.write =functionwrite(chunk,encoding,callback){returnret;};-functiononError(msg,err,callback){-consttriggerAsyncId=msg.socket ? msg.socket[async_id_symbol] : undefined;-defaultTriggerAsyncIdScope(triggerAsyncId,-process.nextTick,-emitErrorNt,-msg,-err,-callback);-}--functionemitErrorNt(msg,err,callback){-callback(err);-if(typeofmsg.emit==='function'&&!msg._closed){-msg.emit('error',err);-}-}-functionwrite_(msg,chunk,encoding,callback,fromEnd){if(typeofcallback!=='function')callback=nop;
@@ -730,11+709,8 @@ functionwrite_(msg,chunk,encoding,callback,fromEnd){}if(err){-if(!msg.destroyed){-onError(msg,err,callback);-}else{-process.nextTick(callback,err);-}+process.nextTick(callback,err);+msg.destroy(err);returnfalse;}
@@ -823,31+799,37 @@ OutgoingMessage.prototype.end=functionend(chunk,encoding,callback){this.socket.cork();}-if(chunk){-if(this.finished){-onError(this,-newERR_STREAM_WRITE_AFTER_END(),-typeofcallback!=='function' ? nop : callback);-returnthis;+if(chunk!==null&&chunk!==undefined)+this.write(chunk,encoding);++leterr;+if(this.writableFinished){+err=newERR_STREAM_ALREADY_FINISHED('end');+}elseif(this.destroyed){+err=newERR_STREAM_DESTROYED('end');+}++if(typeofcallback==='function'){+if(err||this.writableFinished){+process.nextTick(callback,err);+}else{+// TODO (fix): What if error?+this.once('finish',callback);}-write_(this,chunk,encoding,null,true);-}elseif(this.finished){-if(typeofcallback==='function'){-if(!this.writableFinished){-this.on('finish',callback);-}else{-callback(newERR_STREAM_ALREADY_FINISHED('end'));-}+}++if(err){+if(this.socket){+this.socket.uncork();}returnthis;-}elseif(!this._header){+}++if(!this._header){this._contentLength=0;this._implicitHeader();}-if(typeofcallback==='function')-this.once('finish',callback);-constfinish=FunctionPrototypeBind(onFinish,undefined,this);if(this._hasBody&&this.chunkedEncoding){

[ronag]

A little more work:

diff--gita/lib/_http_outgoing.jsb/lib/_http_outgoing.jsindex50ef063241..b2b3137235100644---a/lib/_http_outgoing.js+++b/lib/_http_outgoing.js
@@ -51,10+51,6 @@ const{ Buffer }=require('buffer');constcommon=require('_http_common');constcheckIsHttpToken=common._checkIsHttpToken;constcheckInvalidHeaderChar=common._checkInvalidHeaderChar;-const{-defaultTriggerAsyncIdScope,-symbols: { async_id_symbol }-}=require('internal/async_hooks');const{codes: {ERR_HTTP_HEADERS_SENT,
@@ -689,23+685,6 @@ OutgoingMessage.prototype.write=functionwrite(chunk,encoding,callback){returnret;};-functiononError(msg,err,callback){-consttriggerAsyncId=msg.socket ? msg.socket[async_id_symbol] : undefined;-defaultTriggerAsyncIdScope(triggerAsyncId,-process.nextTick,-emitErrorNt,-msg,-err,-callback);-}--functionemitErrorNt(msg,err,callback){-callback(err);-if(typeofmsg.emit==='function'&&!msg._closed){-msg.emit('error',err);-}-}-functionwrite_(msg,chunk,encoding,callback,fromEnd){if(typeofcallback!=='function')callback=nop;
@@ -730,11+709,8 @@ functionwrite_(msg,chunk,encoding,callback,fromEnd){}if(err){-if(!msg.destroyed){-onError(msg,err,callback);-}else{-process.nextTick(callback,err);-}+process.nextTick(callback,err);+msg.destroy(err);returnfalse;}
@@ -809,13+785,13 @@ functiononFinish(outmsg){outmsg.emit('finish');}-OutgoingMessage.prototype.end=functionend(chunk,encoding,callback){+OutgoingMessage.prototype.end=functionend(chunk,encoding,cb){if(typeofchunk==='function'){-callback=chunk;+cb=chunk;chunk=null;encoding=null;}elseif(typeofencoding==='function'){-callback=encoding;+cb=encoding;encoding=null;}
@@ -823,38+799,46 @@ OutgoingMessage.prototype.end=functionend(chunk,encoding,callback){this.socket.cork();}-if(chunk){-if(this.finished){-onError(this,-newERR_STREAM_WRITE_AFTER_END(),-typeofcallback!=='function' ? nop : callback);-returnthis;+if(chunk!==null&&chunk!==undefined)+this.write(chunk,encoding);++leterr;+if(!this.finished){+if(!this._header){+this._contentLength=0;+this._implicitHeader();}-write_(this,chunk,encoding,null,true);-}elseif(this.finished){-if(typeofcallback==='function'){-if(!this.writableFinished){-this.on('finish',callback);-}else{-callback(newERR_STREAM_ALREADY_FINISHED('end'));-}++constfinish=FunctionPrototypeBind(onFinish,undefined,this);++if(this._hasBody&&this.chunkedEncoding){+this._send('0\r\n'+this._trailer+'\r\n','latin1',finish);+}else{+// Force a flush, HACK.+this._send('','latin1',finish);}-returnthis;-}elseif(!this._header){-this._contentLength=0;-this._implicitHeader();-}-if(typeofcallback==='function')-this.once('finish',callback);+this.finished=true;// aka. WritableState.ended+}elseif(this.writableFinished){+err=newERR_STREAM_ALREADY_FINISHED('end');+}elseif(this.destroyed){+err=newERR_STREAM_DESTROYED('end');+}-constfinish=FunctionPrototypeBind(onFinish,undefined,this);+if(typeofcb==='function'){+if(err||this.writableFinished){+process.nextTick(cb,err);+}else{+// TODO (fix): What if error? See kOnFinished in writable.js.+this.once('finish',cb);+}+}-if(this._hasBody&&this.chunkedEncoding){-this._send('0\r\n'+this._trailer+'\r\n','latin1',finish);-}else{-// Force a flush, HACK.-this._send('','latin1',finish);+if(err){+if(this.socket){+this.socket.uncork();+}+returnthis;}if(this.socket){
@@ -864,14+848,10 @@ OutgoingMessage.prototype.end=functionend(chunk,encoding,callback){}this[kCorked]=0;-this.finished=true;-// There is the first message on the outgoing queue, and we've sent// everything to the socket.debug('outgoing message end.');-if(this.outputData.length===0&&-this.socket&&-this.socket._httpMessage===this){+if(this.outputData.length===0&&this.socket?._httpMessage===this){this._finish();}

[ronag]

Sorry if I made the scope on this task a lot bigger. But some of the question you had kind of lead to one thing and then another... let me know if you wish to collaborate on this.

Basically our goal here is to make http.OutgoingMessage have the same behavior as stream.Writable (as much as possible at least).

[Lxxyx]

Sorry if I made the scope on this task a lot bigger. But some of the question you had kind of lead to one thing and then another... let me know if you wish to collaborate on this.

I wish to collaborate on this, I've learned a lot from this pull request and your guidance, and it's been a lot of fun.

Basically our goal here is to make http.OutgoingMessage have the same behavior as stream.Writable (as much as possible at least).

Should we split it into multiple Pull Request, or just implement it in this pull request?

[ronag]

I wish to collaborate on this, I've learned a lot from this pull request and your guidance, and it's been a lot of fun.

Do you think you can try to apply my suggestion and see if and what test might fail? We should sort those out first.

Then we need to add some new tests. Once the above is done I can take a look and make some TODO's for stuff that needs test and then you can try to make those? I'll help out if you get stuck with any.

[ronag]

Should we split it into multiple Pull Request, or just implement it in this pull request?

I think it's easier to do it here since a lot of the changes are dependent.

[Lxxyx]

5 unit tests failed at local.

Detail

=== release test-http-content-length ===
Path: parallel/test-http-content-length
node:assert:119
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected
{
+ 'transfer-encoding': 'chunked',
- 'content-length': '11',
connection: 'close'
}
at Server.<anonymous> (~/node/test/parallel/test-http-content-length.js:35:14)
at Server.emit (node:events:379:20)
at parserOnIncoming (node:_http_server:926:12)
at HTTPParser.parserOnHeadersComplete (node:_http_common:129:17) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: { connection: 'close', 'transfer-encoding': 'chunked' },
expected: { connection: 'close', 'content-length': '11' },
operator: 'deepStrictEqual'
}
Command: out/Release/node ~/node/test/parallel/test-http-content-length.js
=== release test-http-outgoing-end-multiple ===
Path: parallel/test-http-outgoing-end-multiple
node:assert:119
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected
+ 'ERR_STREAM_ALREADY_FINISHED'
- 'ERR_STREAM_WRITE_AFTER_END'
^
at ~/node/test/parallel/test-http-outgoing-end-multiple.js:7:10
at ~/node/test/common/index.js:377:15
at processTicksAndRejections (node:internal/process/task_queues:80:21) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: 'ERR_STREAM_ALREADY_FINISHED',
expected: 'ERR_STREAM_WRITE_AFTER_END',
operator: 'strictEqual'
}
Command: out/Release/node ~/node/test/parallel/test-http-outgoing-end-multiple.js
=== release test-http-res-write-after-end ===
Path: parallel/test-http-res-write-after-end
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
at mustCall (~/node/test/common/index.js:335:10)
at Proxy.expectsError (~/node/test/common/index.js:551:10)
at Server.<anonymous> (~/node/test/parallel/test-http-res-write-after-end.js:28:26)
at Server.<anonymous> (~/node/test/common/index.js:377:15)
at Server.emit (node:events:379:20)
at parserOnIncoming (node:_http_server:926:12)
at HTTPParser.parserOnHeadersComplete (node:_http_common:129:17)
Command: out/Release/node ~/node/test/parallel/test-http-res-write-after-end.js
=== release test-http-server-response-standalone ===
Path: parallel/test-http-server-response-standalone
node:assert:399
throw err;
^
AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
assert(chunk.toString().endsWith('hello world'))
at Writable.<anonymous> (~/node/test/parallel/test-http-server-response-standalone.js:23:7)
at Writable._write (~/node/test/common/index.js:377:15)
at doWrite (node:internal/streams/writable:414:12)
at clearBuffer (node:internal/streams/writable:573:7)
at Writable.uncork (node:internal/streams/writable:354:7)
at ServerResponse.end (node:_http_outgoing:851:17)
at Object.<anonymous> (~/node/test/parallel/test-http-server-response-standalone.js:34:5)
at Module._compile (node:internal/modules/cjs/loader:1108:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
at Module.load (node:internal/modules/cjs/loader:973:32) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: false,
expected: true,
operator: '=='
}
Command: out/Release/node ~/node/test/parallel/test-http-server-response-standalone.js
=== release test-pipe-outgoing-message-data-emitted-after-ended ===
Path: parallel/test-pipe-outgoing-message-data-emitted-after-ended
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
at mustCall (~/node/test/common/index.js:335:10)
at Proxy.expectsError (~/node/test/common/index.js:551:10)
at ~/node/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js:19:28
at ~/node/test/common/index.js:377:15
at processTicksAndRejections (node:internal/process/task_queues:76:11)
Command: out/Release/node ~/node/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js

[ronag]

5 unit tests failed at local.

Can you push the changes?

Do you want to take a try at fixing the tests?

[Lxxyx]

Can you push the changes?

The code has been pushed.

Do you want to take a try at fixing the tests?

Yes. I would like to try fixing the tests

[Lxxyx]

@ronag Tried to fix the unit test errors(bbafac6), emitErrorNt is required for on('error') and should throw ERR_STREAM_WRITE_AFTER_END earlier.

[Lxxyx]

Close this Pull Request due to outdated code