http: revert #14024 writable is never set to false #15404

mcollina · 2017-09-14T09:24:21Z

Setting writable = false in IncomingMessage.end made some errors
being swallowed in some very popular OSS libraries that we must
support. This commit add some of those use cases, so we avoid further
regressions.

See: #14024
Fixes: #15029

Checklist

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

Affected core subsystem(s)

http

mcollina · 2017-09-14T09:26:00Z

cc @trygve-lie @SimenB @Kasher @gireeshpunathil

mafintosh · 2017-09-14T10:48:18Z

awesome!

jasnell

It would likely make sense to have a known issue test that covers the fix that is being reverted and includes an explanation as to why it is problematic.

mcollina · 2017-09-14T11:56:43Z

@jasnell it is included https://github.com/nodejs/node/pull/15404/files#diff-0957961bda9af181fe86099a4cbd825e.

cjihrig

LGTM with a comment.

cjihrig · 2017-09-14T14:05:59Z

test/parallel/test-http-writable-true-after-close.js

+ external.abort();
+ res.end('Hello World\n');
+ }));
+}).listen(3000);


Can you write this test without hard coded port numbers.

mscdex · 2017-09-14T14:25:28Z

I'm not really convinced this should be reverted. How are users expected to know whether we can safely call .write()/.end(foo) now, given an arbitrary response object? Reach into ._writableState?

jasnell · 2017-09-14T14:28:19Z

This should be reverted because the change broke a userland module that has over 10 million downloads a month. That's not to say the reversion should be permanent, just that we should take a step back and look at another way of addressing the issue that isn't so breaking.

mcollina · 2017-09-14T14:38:15Z

@mscdex this does not relate to Writable, it's on ServerResponse, which does inherit from Stream directly. This is the source of the problem. Making it inheriting from Writable would slow things down by 50% (at least), so that is out of the equation.

Part of the problem is that this is defined as a "legacy stream", and access to _writableState and _readableState is currently used to know how a stream should behave. ServerResponse is missing that state, so we cannot really know what happened.

I'm definitely 👍 for a fix, but this should be reverted before 8 goes to LTS - and we should figure out how we can make ServerResponse more inspectable. These are the relevant lines:

https://github.com/mafintosh/end-of-stream/blob/master/index.js#L24-L26
https://github.com/mafintosh/end-of-stream/blob/master/index.js#L56-L59

I have tried to pursue a different fix for this for more than a month. I could not find a way to keep this in and not break the ecosystem (or our unit tests) in a semver-major way.

mcollina · 2017-09-14T14:48:50Z

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

mcollina · 2017-09-19T17:22:42Z

CI failures are unrelated.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/990/

cc @refack

BridgeAR

Maybe add a comment that we should look into this again as it is not desired behavior?

Setting writable = false in IncomingMessage.end made some errors being swallowed in some very popular OSS libraries that we must support. This commit add some of those use cases to the tests, so we avoid further regressions. We should reevaluate how to set writable = false in IncomingMessage in a way that does not break the ecosystem. See: nodejs#14024 Fixes: nodejs#15029

mcollina · 2017-09-20T09:32:07Z

@BridgeAR I've added that in the commit message.

mcollina · 2017-09-20T09:41:13Z

Landed as 8589c70.

Setting writable = false in IncomingMessage.end made some errors being swallowed in some very popular OSS libraries that we must support. This commit add some of those use cases to the tests, so we avoid further regressions. We should reevaluate how to set writable = false in IncomingMessage in a way that does not break the ecosystem. See: #14024 Fixes: #15029 PR-URL: #15404 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Setting writable = false in IncomingMessage.end made some errors being swallowed in some very popular OSS libraries that we must support. This commit add some of those use cases to the tests, so we avoid further regressions. We should reevaluate how to set writable = false in IncomingMessage in a way that does not break the ecosystem. See: nodejs/node#14024 Fixes: nodejs/node#15029 PR-URL: nodejs/node#15404 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Ref: nodejs#15404 Fixes: nodejs#15505

Ref: #15404 Fixes: #15505 PR-URL: #15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Ref: nodejs#15404 Fixes: nodejs#15505 PR-URL: nodejs#15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Ref: #15404 Fixes: #15505 PR-URL: #15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Ref: nodejs/node#15404 Fixes: nodejs/node#15505 PR-URL: nodejs/node#15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Ref: #15404 Fixes: #15505 PR-URL: #15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-10-16T21:59:06Z

setting as dont-land-on-v6.x as the original never landed in a release

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Sep 14, 2017

mcollina requested a review from jasnell September 14, 2017 09:24

mcollina mentioned this pull request Sep 14, 2017

http: OutgoingMessage change writable after end #14024

Closed

3 tasks

SimenB mentioned this pull request Sep 14, 2017

"premature close" error not thrown in node 8.2.0 or newer mafintosh/end-of-stream#14

Closed

jasnell approved these changes Sep 14, 2017

View reviewed changes

cjihrig approved these changes Sep 14, 2017

View reviewed changes

SimenB mentioned this pull request Sep 14, 2017

Possible bug in streams between node 8.1.4 and 8.2.0 #15029

Closed

mcollina force-pushed the fix-15029 branch 2 times, most recently from 2d39825 to c0eb9f8 Compare September 14, 2017 14:48

BridgeAR approved these changes Sep 19, 2017

View reviewed changes

mcollina force-pushed the fix-15029 branch from c0eb9f8 to cccf934 Compare September 20, 2017 09:31

mcollina closed this Sep 20, 2017

mcollina deleted the fix-15029 branch September 20, 2017 09:41

mhdawson mentioned this pull request Sep 20, 2017

test-http-writable-true-after-close failure on centos #15505

Closed

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

mcollina mentioned this pull request Sep 21, 2017

test: defer abort with setTimeout #15520

Closed

2 tasks

mcollina added a commit to mcollina/node that referenced this pull request Sep 27, 2017

test: fix http-writable-true-after-close flakyness

dfd4c63

Ref: nodejs#15404 Fixes: nodejs#15505

MylesBorins added the dont-land-on-v6.x label Oct 16, 2017

apapirovski mentioned this pull request Apr 23, 2018

reevaluate how to set writable = false in IncomingMessage #20233

Closed

ronag mentioned this pull request Jul 16, 2019

http: writable set to false #28715

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: revert #14024 writable is never set to false #15404

http: revert #14024 writable is never set to false #15404

mcollina commented Sep 14, 2017

mcollina commented Sep 14, 2017

mafintosh commented Sep 14, 2017

jasnell left a comment

mcollina commented Sep 14, 2017

cjihrig left a comment

cjihrig Sep 14, 2017

mscdex commented Sep 14, 2017

jasnell commented Sep 14, 2017

mcollina commented Sep 14, 2017 •

edited

Loading

mcollina commented Sep 14, 2017

mcollina commented Sep 19, 2017 •

edited

Loading

BridgeAR left a comment

mcollina commented Sep 20, 2017

mcollina commented Sep 20, 2017

MylesBorins commented Oct 16, 2017

http: revert #14024 writable is never set to false #15404

http: revert #14024 writable is never set to false #15404

Conversation

mcollina commented Sep 14, 2017

Checklist

Affected core subsystem(s)

mcollina commented Sep 14, 2017

mafintosh commented Sep 14, 2017

jasnell left a comment

Choose a reason for hiding this comment

mcollina commented Sep 14, 2017

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Sep 14, 2017

Choose a reason for hiding this comment

mscdex commented Sep 14, 2017

jasnell commented Sep 14, 2017

mcollina commented Sep 14, 2017 • edited Loading

mcollina commented Sep 14, 2017

mcollina commented Sep 19, 2017 • edited Loading

BridgeAR left a comment

Choose a reason for hiding this comment

mcollina commented Sep 20, 2017

mcollina commented Sep 20, 2017

MylesBorins commented Oct 16, 2017

mcollina commented Sep 14, 2017 •

edited

Loading

mcollina commented Sep 19, 2017 •

edited

Loading