stream: delete redundant code by MoonBall · Pull Request #18145 · nodejs/node

MoonBall · 2018-01-14T16:01:28Z

In Writable.prototype.end(), state.ending is true after calling
endWritable() and it doesn't reset to false.

In Writable.prototype.uncork(), state.finished must be false
if state.bufferedRequest is true.

In onCorkedFinish(), state.corkedRequestsFree of a writable
stream is never null.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

stream

In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true.

`state.corkedRequestsFree` of a writable stream is always not null.

mcollina

Thanks for contributing! I've left some questions/notes around the PR.

mcollina · 2018-01-17T12:18:54Z

- }
+
+ // reuse the free corkReq.
+ state.corkedRequestsFree.next = corkReq;


Unfortunately, this can end up being null because it copied over in: https://github.com/MoonBall/node/blob/dd13a79b830ef2d33ac0288e197a431a63e40941/lib/_stream_writable.js#L517. Can you revert this?

if (holder.next) can ensure that state.corkedRequestsFree is not null.

My bad. I'm very scared of #6164 and #6154

mcollina · 2018-01-17T12:19:52Z


 // ignore unnecessary end() calls.
- if (!state.ending && !state.finished)
+ if (!state.ending)


This is actually a behavior change. Can you add a unit test?

state.finished = true is only in finishMaybe(). If we simplify the finishMaybe(), it should be:
function finishMaybe(stream, state) { if (state.ending) { state.finished = true; } }
And, state.ending doesn't set to false.
So:
if state.finished is true, state.ending must be true:
!state.ending && !state.finished equals false && false.
!state.ending equals false.
!state.ending && !state.finished equals !state.ending.
if state.finished is false:
!state.ending && !state.finished equals !state.ending && true.
!state.ending && !state.finished equals !state.ending.

mcollina · 2018-01-17T12:20:37Z


 if (!state.writing &&
 !state.corked &&
- !state.finished &&


why is this unused?

state.finished should equal false if state.bufferedRequest isn't null.

mcollina · 2018-01-17T17:19:04Z

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

BridgeAR · 2018-02-01T17:29:37Z

@mcollina PTAL

mcollina · 2018-02-01T17:53:57Z

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

mcollina

LGTM

mcollina · 2018-02-02T09:35:41Z

Landed as ce83099 and fca1c55

In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins · 2018-03-20T16:18:04Z

Should this land on LTS? It lands cleanly on 8.x, but will require a backport for 6.x

In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true. PR-URL: nodejs#18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: nodejs#18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins · 2018-05-23T18:54:17Z

I've landed on 8.x @nodejs/streams @mcollina please lmk if it should be backed out

In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

mcollina · 2018-05-23T19:26:22Z

This can be backported.

In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

stream: delete redundant code
4ecb688
In `Writable.prototype.end()`, `state.ending` is true after calling `endWritable()` and it doesn't reset to false. In `Writable.prototype.uncork()`, `state.finished` must be false if `state.bufferedRequest` is true.

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 14, 2018

jasnell requested a review from mcollina January 14, 2018 21:42

stream: delete redundant code
dd13a79
`state.corkedRequestsFree` of a writable stream is always not null.

mcollina reviewed Jan 17, 2018
View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952Compare January 26, 2018 22:36

BridgeAR approved these changes Feb 1, 2018
View reviewed changes

mcollina approved these changes Feb 2, 2018
View reviewed changes

mcollina closed this Feb 2, 2018

MylesBorins mentioned this pull request Feb 21, 2018
v9.6.0 proposal #18902
Merged

MylesBorins added lts-watch-v6.x labels Mar 20, 2018

MylesBorins added land-on-v8.x and removed lts-watch-v6.x labels May 23, 2018

MylesBorins mentioned this pull request Jul 9, 2018
v8.12.0 proposal #21593
Merged

Uh oh!

Uh oh!

Conversation

MoonBall commented Jan 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollinaJan 17, 2018

Choose a reason for hiding this comment

Uh oh!

MoonBallJan 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollinaJan 17, 2018

Choose a reason for hiding this comment

Uh oh!

mcollinaJan 17, 2018

Choose a reason for hiding this comment

Uh oh!

MoonBallJan 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollinaJan 17, 2018

Choose a reason for hiding this comment

Uh oh!

MoonBallJan 17, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jan 17, 2018

Uh oh!

BridgeAR commented Feb 1, 2018

Uh oh!

mcollina commented Feb 1, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Feb 2, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

MylesBorins commented May 23, 2018

Uh oh!

mcollina commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MoonBall commented Jan 14, 2018•
edited
Loading

MoonBallJan 17, 2018•
edited
Loading

MoonBallJan 17, 2018•
edited
Loading