http: emit ERR_STREAM_DESTROYED if the socket is destroyed #29227

mcollina · 2019-08-20T14:35:57Z

The pipeline utility assumes that a stream is going to error if it
could not be written to it. If an http.OutgoingMessage was piped after
the underlining socket had been destroyed, the whole pipeline would not
be teared down, resulting in a file descriptor and memory leak.

Checklist

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

mcollina · 2019-08-20T14:49:17Z

cc @ronag @Trott

BridgeAR · 2019-08-20T15:02:36Z

test/parallel/test-http-outgoing-socket-destroyed.js

+    });
+
+    req.on('response', common.mustNotCall());
+    req.on('error', common.mustCall());


If I am not mistaken, this results in the ERR_STREAM_DESTROYED as well?

Suggested change

req.on('error', common.mustCall());

req.on('error', common.expectsError({

code: 'ERR_STREAM_DESTROYED'

}));

ping @mcollina Does that suggestion above seem good to you? (Seems good to me....)

That's going to be 'ECONNRESET' on my machine.

BridgeAR · 2019-08-20T15:02:36Z

Sign in to view

+      callback(err);
+    }
+    if (this.listenerCount('error') > 0 && this[kErrored] === undefined) {
+      this[kErrored] = true;


Is there a specific reason to silence all errors besides the first one?

@BridgeAR: that's how streams do it, see destroy.js.

The reason is that this will normally be called 4 times in case this condition is met, as a response is composed of 4 chunks.

ronag

Should probably also apply the kErrored logic got writeAfterEndNT, and maybe _implicitHeader and pipe. Maybe refactored out into a helper along the lines of errorOrDestroy?

ronag · 2019-08-20T15:04:58Z

Sign in to view

+    if (callback) {
+      callback(err);
+    }
+    if (this.listenerCount('error') > 0 && this[kErrored] === undefined) {


What's the reason for the listener count check? We don't do this for streams?

nit, !this[kErrored]

The reason is not to break a lot of tests, and keep this semver-minor.

Could you add a comment for that at least? Then maybe we can "fix" it in a semver-major in the future?

ronag · 2019-08-20T15:04:58Z

Sign in to view

@@ -56,6 +57,7 @@ const HIGH_WATER_MARK = getDefaultHighWaterMark();
 const { CRLF, debug } = common;

 const kIsCorked = Symbol('isCorked');
+const kErrored = Symbol('errored');


nit kErrorEmitted to more closely follow streams

Should we initialize kErrored in the constructor? Or will that unnecessarily increase object size?

I'm not adding that to the constructor exactly because of that. I can add it if we think it's worth adding, it's not a big deal anyway.

Sounds good either way to me. Just checking if it was intentional.

ronag · 2019-08-20T15:04:58Z

Sign in to view

+    }
+    if (this.listenerCount('error') > 0 && this[kErrored] === undefined) {
+      this[kErrored] = true;
+      this.emit('error', err);


I believe this should be emitted in next tick? That's what we want in stream?

Maybe same thing with callback?

I'm actually removing calling the callback here. It's not needed to fix the actual issue, and it can be done in a later step.

We cannot remove the callback or something else is going to fail. Essentially if we want to emit error here, we need to call the callback synchronously first :/.

Something else, as in a test? Add another comment for future semver-major?

mcollina · 2019-08-20T15:24:12Z

Should probably also apply the kErrored logic got writeAfterEndNT, and maybe _implicitHeader and pipe. Maybe refactored out into a helper along the lines of errorOrDestroy?

I'm not sure about that. The reason I added kErrored was because otherwise we would have 4 'error' events instead of 1 for normal operations.

ronag · 2019-08-20T15:32:09Z

I'm not sure about that.

Could you explain a little further? In Writable we only emit one error? The only reason (as far as I understand) that this is not a Writable is performance concerns. So in my opinion this should behave as similar as possible without affecting performance.

mcollina · 2019-08-20T15:34:15Z

The overall reason for not doing that in this PR is to keep it as strictly semver-minor as possible, so it can be backported.

ronag · 2019-08-20T15:35:45Z

The overall reason for not doing that in this PR is to keep it as strictly semver-minor as possible, so it can be backported.

Sounds reasonable. I'm willing to make a semver-major PR in the future to make it more stream-like.

lib/_http_outgoing.js

Trott · 2019-08-23T04:30:46Z

Rebased and force-pushed to get rid of conflict.

nodejs-github-bot · 2019-08-23T04:31:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25195/

nodejs-github-bot · 2019-08-23T09:40:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25202/

nodejs-github-bot · 2019-08-23T12:32:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25203/

test/parallel/test-http-outgoing-socket-destroyed.js

nodejs-github-bot · 2019-08-23T13:10:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25204/

mcollina · 2019-08-23T14:30:57Z

CI is not flaky, windows is failing :/

Mismatched <anonymous> function calls. Expected exactly 2, actual 1.
    at Object.mustCall (c:\workspace\node-test-binary-windows-2\test\common\index.js:337:10)
    at Server.<anonymous> (c:\workspace\node-test-binary-windows-2\test\parallel\test-http-destroyed-socket-write2.js:52:26)
    at Object.onceWrapper (events.js:297:20)
    at Server.emit (events.js:209:13)
    at emitListeningNT (net.js:1262:10)
    at processTicksAndRejections (internal/process/task_queues.js:77:11)

jasnell · 2020-07-07T15:02:13Z

Ping @mcollina and @ronag

mcollina · 2020-07-08T08:14:44Z

@ronag if you would like to take this over please do.

github-actions · 2020-10-19T17:40:16Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

mcollina · 2020-10-23T11:31:49Z

@ronag wdyt? Should we revive this or should I close it?

ronag · 2020-10-23T11:49:14Z

@ronag wdyt? Should we revive this or should I close it?

@mcollina I'm on vacation next month so I hope to cleanup some of these. Thanks for the ping!

ronag · 2020-12-29T11:38:03Z

@mcollina: I think this PR is no longer making correct assumptions. Streams do not emit ERR_STREAM_DESTROYED (at least not anymore) and will only provide it as an error to the callback.

If the underlying socket is prematurely destroyed (without error) that should already error the OutgoingMessage. Though, I'm not sure whether that is actually the case for ServerResponse?

What we should probably fix is:

Make sure the callback is invoked with ERR_STREAM_DESTROYED.
Have pipeline check during invocation whether any of the streams already is .destroyed and error the pipeline with ERR_STREAM_DESTROYED.

I would recommend we close this PR and make an issue for the callback and pipeline issue. Thoughts?

mcollina · 2020-12-29T12:03:18Z

I'll close this. Could you open the issue?

Fixes: nodejs#36674 Refs: nodejs#29227 (comment)

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

mcollina requested review from mscdex, jasnell, addaleax, lpinca and BridgeAR Aug 20, 2019

nodejs-github-bot added the http label Aug 20, 2019

BridgeAR approved these changes Aug 20, 2019

View changes

ronag requested changes Aug 20, 2019

View changes

mcollina force-pushed the mcollina:http-outgoing-destroyed branch Aug 20, 2019

ronag approved these changes Aug 20, 2019

View changes

ronag reviewed Aug 20, 2019

View changes

lib/_http_outgoing.js Show resolved Hide resolved

ronag requested changes Aug 20, 2019

View changes

lib/_http_outgoing.js Show resolved Hide resolved

Trott force-pushed the mcollina:http-outgoing-destroyed branch Aug 23, 2019

mcollina force-pushed the mcollina:http-outgoing-destroyed branch 2 times, most recently Aug 23, 2019

ronag approved these changes Aug 23, 2019

View changes

mcollina force-pushed the mcollina:http-outgoing-destroyed branch Aug 23, 2019

lpinca approved these changes Aug 23, 2019

View changes

test/parallel/test-http-outgoing-socket-destroyed.js Show resolved Hide resolved

jasnell approved these changes Aug 23, 2019

View changes

BridgeAR force-pushed the nodejs:master branch 2 times, most recently from 8ae28ff to 2935f72 May 31, 2020

jasnell added the stalled label Jul 7, 2020

mcollina requested a review from nodejs/net as a code owner Aug 10, 2020

aduh95 added stalled and removed stalled labels Oct 19, 2020

ronag self-assigned this Dec 27, 2020

ronag removed their assignment Dec 29, 2020

mcollina closed this Dec 29, 2020

This was referenced Dec 29, 2020

http: OutgoingMessage ERR_STREAM_DESTROYED callback #36673

Open

stream: pipeline should error if any stream is destroyed #36674

Open

Lxxyx mentioned this pull request Dec 30, 2020

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

Open

3 of 3 tasks complete

kalenikalexander pushed a commit to kalenikalexander/node that referenced this pull request Jan 4, 2021

streams: pipeline error if any stream is destroyed

b934636

Fixes: nodejs#36674 Refs: nodejs#29227 (comment)

kalenikalexander mentioned this pull request Jan 4, 2021

streams: pipeline error if any stream is destroyed #36791

Open

2 of 2 tasks complete

Lxxyx added a commit to Lxxyx/node that referenced this pull request Jan 12, 2021

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

2a0ecd6

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

-    req.on('error', common.mustCall());
+    req.on('error', common.expectsError({
+      code: 'ERR_STREAM_DESTROYED'
+    }));

nodejs / node

http: emit ERR_STREAM_DESTROYED if the socket is destroyed #29227

http: emit ERR_STREAM_DESTROYED if the socket is destroyed #29227

Conversation

mcollina commented Aug 20, 2019

Checklist

mcollina commented Aug 20, 2019

This comment has been minimized.

BridgeAR Aug 20, 2019 Member

This comment has been minimized.

Trott Aug 23, 2019 Member

This comment has been minimized.

mcollina Aug 23, 2019 Author Member

This comment has been minimized.

BridgeAR Aug 20, 2019 Member

This comment has been minimized.

ronag Aug 20, 2019 Member

This comment has been minimized.

mcollina Aug 20, 2019 Author Member

ronag left a comment • edited

This comment has been minimized.

ronag Aug 20, 2019 Member

This comment has been minimized.

ronag Aug 20, 2019 Member

This comment has been minimized.

mcollina Aug 20, 2019 Author Member

This comment has been minimized.

ronag Aug 20, 2019 Member

This comment has been minimized.

ronag Aug 20, 2019 Member

This comment has been minimized.

ronag Aug 20, 2019 • edited Member

This comment has been minimized.

mcollina Aug 20, 2019 Author Member

This comment has been minimized.

ronag Aug 20, 2019 Member

This comment has been minimized.

ronag Aug 20, 2019 • edited Member

This comment has been minimized.

ronag Aug 20, 2019 Member

This comment has been minimized.

mcollina Aug 20, 2019 Author Member

This comment has been minimized.

mcollina Aug 20, 2019 Author Member

This comment has been minimized.

ronag Aug 20, 2019 • edited Member

mcollina commented Aug 20, 2019

ronag commented Aug 20, 2019 • edited

mcollina commented Aug 20, 2019

ronag commented Aug 20, 2019

Trott commented Aug 23, 2019

nodejs-github-bot commented Aug 23, 2019

nodejs-github-bot commented Aug 23, 2019

nodejs-github-bot commented Aug 23, 2019

nodejs-github-bot commented Aug 23, 2019

mcollina commented Aug 23, 2019

jasnell commented Jul 7, 2020

mcollina commented Jul 8, 2020

github-actions bot commented Oct 19, 2020

mcollina commented Oct 23, 2020

ronag commented Oct 23, 2020

ronag commented Dec 29, 2020

mcollina commented Dec 29, 2020

BridgeAR Aug 20, 2019
Member

Trott Aug 23, 2019
Member

mcollina Aug 23, 2019
Author Member

BridgeAR Aug 20, 2019
Member

ronag Aug 20, 2019
Member

mcollina Aug 20, 2019
Author Member

ronag left a comment •

edited

ronag Aug 20, 2019
Member

ronag Aug 20, 2019
Member

mcollina Aug 20, 2019
Author Member

ronag Aug 20, 2019
Member

ronag Aug 20, 2019
Member

ronag Aug 20, 2019 •

edited

Member

mcollina Aug 20, 2019
Author Member

ronag Aug 20, 2019
Member

ronag Aug 20, 2019 •

edited

Member

ronag Aug 20, 2019
Member

mcollina Aug 20, 2019
Author Member

mcollina Aug 20, 2019
Author Member

ronag Aug 20, 2019 •

edited

Member

ronag commented Aug 20, 2019 •

edited