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-20T14:58:04Z

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:01:31Z

lib/_http_outgoing.js

+      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:00:07Z

lib/_http_outgoing.js

+    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:01:35Z

lib/_http_outgoing.js

@@ -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:12Z

lib/_http_outgoing.js

+    }
+    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 August 20, 2019 14:35

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 20, 2019

BridgeAR approved these changes Aug 20, 2019

View reviewed changes

ronag requested changes Aug 20, 2019

View reviewed changes

mcollina force-pushed the http-outgoing-destroyed branch from 44b241f to 28a771d Compare August 20, 2019 15:24

ronag approved these changes Aug 20, 2019

View reviewed changes

ronag reviewed Aug 20, 2019

View reviewed changes

lib/_http_outgoing.js Show resolved Hide resolved

ronag requested changes Aug 20, 2019

View reviewed changes

lib/_http_outgoing.js Show resolved Hide resolved

Trott force-pushed the http-outgoing-destroyed branch from 55dc8b6 to 40c62a6 Compare August 23, 2019 04:30

mcollina force-pushed the http-outgoing-destroyed branch 2 times, most recently from e2f7aec to d2cfed1 Compare August 23, 2019 07:08

ronag approved these changes Aug 23, 2019

View reviewed changes

mcollina force-pushed the http-outgoing-destroyed branch from eca886a to a085d91 Compare August 23, 2019 12:31

lpinca approved these changes Aug 23, 2019

View reviewed changes

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

jasnell approved these changes Aug 23, 2019

View reviewed changes

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

jasnell added the stalled Issues and PRs that are stalled. label Jul 7, 2020

mcollina requested a review from a team as a code owner August 10, 2020 16:08

aduh95 added stalled Issues and PRs that are stalled. and removed stalled Issues and PRs that are 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

Closed

Lxxyx mentioned this pull request Dec 30, 2020

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

Closed

3 tasks

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

Closed

2 tasks

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'
+    }));

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

mcollina commented Aug 20, 2019

ronag commented Aug 20, 2019 • edited Loading

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

ronag left a comment •

edited

Loading

ronag Aug 20, 2019 •

edited

Loading

ronag Aug 20, 2019 •

edited

Loading

ronag Aug 20, 2019 •

edited

Loading

ronag commented Aug 20, 2019 •

edited

Loading