http: revert #14024 writable is never set to false#15404
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mcollina commented Sep 14, 2017
mafintosh commented Sep 14, 2017
awesome! |
jasnell left a comment
jasnell left a comment There was a problem hiding this comment.
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 commented Sep 14, 2017
There was a problem hiding this comment.
Can you write this test without hard coded port numbers.
mscdex commented Sep 14, 2017
I'm not really convinced this should be reverted. How are users expected to know whether we can safely call |
jasnell commented Sep 14, 2017
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. |
@mscdex this does not relate to Part of the problem is that this is defined as a "legacy stream", and access to 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 https://github.com/mafintosh/end-of-stream/blob/master/index.js#L24-L26 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. |
mcollinaforce-pushed the fix-15029 branch 2 times, most recently from 2d39825 to c0eb9f8Comparemcollina commented Sep 14, 2017
CI failures are unrelated. CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/990/ cc @refack |
BridgeAR left a comment
BridgeAR left a comment There was a problem hiding this comment.
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#14024Fixes: nodejs#15029
mcollina commented Sep 20, 2017
@BridgeAR I've added that in the commit message. |
mcollina commented Sep 20, 2017
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: #14024Fixes: #15029 PR-URL: #15404 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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: #14024Fixes: #15029 PR-URL: #15404 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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#14024Fixes: nodejs/node#15029 PR-URL: nodejs/node#15404 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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#14024Fixes: nodejs/node#15029 PR-URL: nodejs/node#15404 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Ref: #15404Fixes: #15505 PR-URL: #15520 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Ref: nodejs#15404Fixes: nodejs#15505 PR-URL: nodejs#15520 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Ref: #15404Fixes: #15505 PR-URL: #15520 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Ref: nodejs/node#15404Fixes: nodejs/node#15505 PR-URL: nodejs/node#15520 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Ref: #15404Fixes: #15505 PR-URL: #15520 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins commented Oct 16, 2017
setting as dont-land-on-v6.x as the original never landed in a release |
8 participants
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), orvcbuild test(Windows) passesAffected core subsystem(s)
http