http: invoke callback with ERR_STREAM_DESTROYED if the socket is destroyed #36692

Lxxyx · 2020-12-30T14:36:41Z

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 · 2021-01-05T12:14:08Z

ping @ronag

lib/_http_outgoing.js

ronag

commit description is wrong, should be invoke callback with ERR_STREAM_DESTROYED

lib/_http_outgoing.js

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.

lib/_http_outgoing.js

Lxxyx · 2021-01-05T18:31:04Z

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 · 2021-01-05T18:32:32Z

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 · 2021-01-06T05:58:02Z

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 · 2021-01-06T12:49:49Z

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

    
           let err; 
        
           if (msg.finished) { 
        
             err = new ERR_STREAM_WRITE_AFTER_END(); 
        
           } else if (msg.destroyed) { 
        
             err = new ERR_STREAM_DESTROYED('write'); 
        
           }

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

ronag · 2021-01-06T13:08:17Z

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

diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js
index 50ef063241..6e6ab8eb92 100644
--- a/lib/_http_outgoing.js
+++ b/lib/_http_outgoing.js
@@ -51,10 +51,6 @@ const { Buffer } = require('buffer');
 const common = require('_http_common');
 const checkIsHttpToken = common._checkIsHttpToken;
 const checkInvalidHeaderChar = 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 = function write(chunk, encoding, callback) {
   return ret;
 };
 
-function onError(msg, err, callback) {
-  const triggerAsyncId = msg.socket ? msg.socket[async_id_symbol] : undefined;
-  defaultTriggerAsyncIdScope(triggerAsyncId,
-                             process.nextTick,
-                             emitErrorNt,
-                             msg,
-                             err,
-                             callback);
-}
-
-function emitErrorNt(msg, err, callback) {
-  callback(err);
-  if (typeof msg.emit === 'function' && !msg._closed) {
-    msg.emit('error', err);
-  }
-}
-
 function write_(msg, chunk, encoding, callback, fromEnd) {
   if (typeof callback !== 'function')
     callback = nop;
@@ -730,11 +709,8 @@ function write_(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);
     return false;
   }
 
@@ -823,31 +799,37 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
     this.socket.cork();
   }
 
-  if (chunk) {
-    if (this.finished) {
-      onError(this,
-              new ERR_STREAM_WRITE_AFTER_END(),
-              typeof callback !== 'function' ? nop : callback);
-      return this;
+  if (chunk !== null && chunk !== undefined)
+    this.write(chunk, encoding);
+
+  let err;
+  if (this.writableFinished) {
+    err = new ERR_STREAM_ALREADY_FINISHED('end');
+  } else if (this.destroyed) {
+    err = new ERR_STREAM_DESTROYED('end');
+  }
+
+  if (typeof callback === '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);
-  } else if (this.finished) {
-    if (typeof callback === 'function') {
-      if (!this.writableFinished) {
-        this.on('finish', callback);
-      } else {
-        callback(new ERR_STREAM_ALREADY_FINISHED('end'));
-      }
+  }
+
+  if (err) {
+    if (this.socket) {
+      this.socket.uncork();
     }
     return this;
-  } else if (!this._header) {
+  }
+
+  if (!this._header) {
     this._contentLength = 0;
     this._implicitHeader();
   }
 
-  if (typeof callback === 'function')
-    this.once('finish', callback);
-
   const finish = FunctionPrototypeBind(onFinish, undefined, this);
 
   if (this._hasBody && this.chunkedEncoding) {

ronag · 2021-01-06T13:51:55Z

A little more work:

diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js
index 50ef063241..b2b3137235 100644
--- a/lib/_http_outgoing.js
+++ b/lib/_http_outgoing.js
@@ -51,10 +51,6 @@ const { Buffer } = require('buffer');
 const common = require('_http_common');
 const checkIsHttpToken = common._checkIsHttpToken;
 const checkInvalidHeaderChar = 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 = function write(chunk, encoding, callback) {
   return ret;
 };
 
-function onError(msg, err, callback) {
-  const triggerAsyncId = msg.socket ? msg.socket[async_id_symbol] : undefined;
-  defaultTriggerAsyncIdScope(triggerAsyncId,
-                             process.nextTick,
-                             emitErrorNt,
-                             msg,
-                             err,
-                             callback);
-}
-
-function emitErrorNt(msg, err, callback) {
-  callback(err);
-  if (typeof msg.emit === 'function' && !msg._closed) {
-    msg.emit('error', err);
-  }
-}
-
 function write_(msg, chunk, encoding, callback, fromEnd) {
   if (typeof callback !== 'function')
     callback = nop;
@@ -730,11 +709,8 @@ function write_(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);
     return false;
   }
 
@@ -809,13 +785,13 @@ function onFinish(outmsg) {
   outmsg.emit('finish');
 }
 
-OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
+OutgoingMessage.prototype.end = function end(chunk, encoding, cb) {
   if (typeof chunk === 'function') {
-    callback = chunk;
+    cb = chunk;
     chunk = null;
     encoding = null;
   } else if (typeof encoding === 'function') {
-    callback = encoding;
+    cb = encoding;
     encoding = null;
   }
 
@@ -823,38 +799,46 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
     this.socket.cork();
   }
 
-  if (chunk) {
-    if (this.finished) {
-      onError(this,
-              new ERR_STREAM_WRITE_AFTER_END(),
-              typeof callback !== 'function' ? nop : callback);
-      return this;
+  if (chunk !== null && chunk !== undefined)
+    this.write(chunk, encoding);
+
+  let err;
+  if (!this.finished) {
+    if (!this._header) {
+      this._contentLength = 0;
+      this._implicitHeader();
     }
-    write_(this, chunk, encoding, null, true);
-  } else if (this.finished) {
-    if (typeof callback === 'function') {
-      if (!this.writableFinished) {
-        this.on('finish', callback);
-      } else {
-        callback(new ERR_STREAM_ALREADY_FINISHED('end'));
-      }
+
+    const finish = 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);
     }
-    return this;
-  } else if (!this._header) {
-    this._contentLength = 0;
-    this._implicitHeader();
-  }
 
-  if (typeof callback === 'function')
-    this.once('finish', callback);
+    this.finished = true; // aka. WritableState.ended
+  } else if (this.writableFinished) {
+    err = new ERR_STREAM_ALREADY_FINISHED('end');
+  } else if (this.destroyed) {
+    err = new ERR_STREAM_DESTROYED('end');
+  }
 
-  const finish = FunctionPrototypeBind(onFinish, undefined, this);
+  if (typeof cb === '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();
+    }
+    return this;
   }
 
   if (this.socket) {
@@ -864,14 +848,10 @@ OutgoingMessage.prototype.end = function end(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 · 2021-01-06T13:52:30Z

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 · 2021-01-06T14:03:27Z

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 · 2021-01-06T14:09:36Z

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 · 2021-01-06T14:10:05Z

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 · 2021-01-06T14:58:57Z

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 · 2021-01-06T15:07:14Z

5 unit tests failed at local.

Can you push the changes?

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

Lxxyx · 2021-01-06T15:27:33Z

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

…royed Fixes: nodejs#36673 Refs: nodejs#29227 (comment)

Lxxyx · 2021-01-12T13:20:38Z

@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 · 2021-05-03T06:50:53Z

Close this Pull Request due to outdated code

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Dec 30, 2020

PoojaDurgad added the stream Issues and PRs related to the stream subsystem. label Jan 1, 2021

ronag reviewed Jan 5, 2021

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

ronag reviewed Jan 5, 2021

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

ronag requested changes Jan 5, 2021

View reviewed changes

Lxxyx force-pushed the http-outgoing-emit-error branch from a758806 to 9ebd6c3 Compare January 5, 2021 16:47

Lxxyx changed the title ~~http: emit ERR_STREAM_DESTROYED if the socket is destroyed~~ http: invoke callback with ERR_STREAM_DESTROYED if the socket is destroyed Jan 5, 2021

Lxxyx requested a review from ronag January 5, 2021 16:49

ronag reviewed Jan 5, 2021

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

Lxxyx force-pushed the http-outgoing-emit-error branch from 9ebd6c3 to 2c2d3c3 Compare January 5, 2021 17:35

ronag reviewed Jan 5, 2021

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

Lxxyx force-pushed the http-outgoing-emit-error branch from 2c2d3c3 to c185bc3 Compare January 5, 2021 17:44

This comment has been minimized.

Sign in to view

Lxxyx force-pushed the http-outgoing-emit-error branch from c185bc3 to 92e2750 Compare January 5, 2021 17:55

ronag approved these changes Jan 5, 2021

View reviewed changes

ronag requested changes Jan 5, 2021

View reviewed changes

Lxxyx force-pushed the http-outgoing-emit-error branch from 92e2750 to 110047e Compare January 5, 2021 18:20

ronag reviewed Jan 5, 2021

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

ronag reviewed Jan 5, 2021

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

Lxxyx force-pushed the http-outgoing-emit-error branch from 110047e to 9024f44 Compare January 6, 2021 12:41

Lxxyx force-pushed the http-outgoing-emit-error branch from 9024f44 to 4ebf579 Compare January 6, 2021 15:25

Lxxyx added 2 commits January 12, 2021 18:38

http: invoke callback with ERR_STREAM_DESTROYED if the socket is dest…

2a0ecd6

…royed Fixes: nodejs#36673 Refs: nodejs#29227 (comment)

fixup! unit test errors

bbafac6

Lxxyx force-pushed the http-outgoing-emit-error branch from 4ebf579 to bbafac6 Compare January 12, 2021 11:10

Lxxyx requested a review from ronag January 12, 2021 12:54

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Lxxyx closed this May 3, 2021

Lxxyx deleted the http-outgoing-emit-error branch May 3, 2021 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: invoke callback with ERR_STREAM_DESTROYED if the socket is destroyed #36692

http: invoke callback with ERR_STREAM_DESTROYED if the socket is destroyed #36692

Lxxyx commented Dec 30, 2020

Lxxyx commented Jan 5, 2021

ronag left a comment

This comment has been minimized.

ronag left a comment

Lxxyx commented Jan 5, 2021

ronag commented Jan 5, 2021 •

edited

Lxxyx commented Jan 6, 2021 •

edited

Lxxyx commented Jan 6, 2021

ronag commented Jan 6, 2021 •

edited

ronag commented Jan 6, 2021

ronag commented Jan 6, 2021 •

edited

Lxxyx commented Jan 6, 2021

ronag commented Jan 6, 2021 •

edited

ronag commented Jan 6, 2021

Lxxyx commented Jan 6, 2021

ronag commented Jan 6, 2021

Lxxyx commented Jan 6, 2021

Lxxyx commented Jan 12, 2021

Lxxyx commented May 3, 2021

http: invoke callback with ERR_STREAM_DESTROYED if the socket is destroyed #36692

http: invoke callback with ERR_STREAM_DESTROYED if the socket is destroyed #36692

Conversation

Lxxyx commented Dec 30, 2020

Checklist

Lxxyx commented Jan 5, 2021

ronag left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ronag left a comment

Choose a reason for hiding this comment

Lxxyx commented Jan 5, 2021

ronag commented Jan 5, 2021 • edited

Lxxyx commented Jan 6, 2021 • edited

Lxxyx commented Jan 6, 2021

ronag commented Jan 6, 2021 • edited

ronag commented Jan 6, 2021

ronag commented Jan 6, 2021 • edited

Lxxyx commented Jan 6, 2021

ronag commented Jan 6, 2021 • edited

ronag commented Jan 6, 2021

Lxxyx commented Jan 6, 2021

ronag commented Jan 6, 2021

Lxxyx commented Jan 6, 2021

Lxxyx commented Jan 12, 2021

Lxxyx commented May 3, 2021

ronag commented Jan 5, 2021 •

edited

Lxxyx commented Jan 6, 2021 •

edited

ronag commented Jan 6, 2021 •

edited

ronag commented Jan 6, 2021 •

edited

ronag commented Jan 6, 2021 •

edited