stream: delete redundant code #18145

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

lib/_stream_writable.js

-  }
+
+  // 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

lib/_stream_writable.js


  // 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

lib/_stream_writable.js


    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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: nodejs#18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare 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

mcollina pushed a commit that referenced this pull request Feb 2, 2018

stream: delete redundant code

fca1c55

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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

rvagg pushed a commit that referenced this pull request Aug 16, 2018

stream: delete redundant code

c92d66a

`state.corkedRequestsFree` of a writable stream is always not null. PR-URL: #18145 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Uh oh!

stream: delete redundant code #18145

stream: delete redundant code #18145

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!

mcollina Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

MoonBall Jan 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!

mcollina Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

MoonBall Jan 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!

mcollina Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

MoonBall Jan 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

MoonBall Jan 17, 2018 •

edited

Loading

MoonBall Jan 17, 2018 •

edited

Loading