FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

stream: delete redundant code - #18145

Closed
MoonBall wants to merge 2 commits into
nodejs:masterfrom
MoonBall:delete-redundant-code-in-Stream.Writable
ConversationCommits (2)ChecksFiles changed
Closed

stream: delete redundant code#18145
MoonBall wants to merge 2 commits into
nodejs:masterfrom
MoonBall:delete-redundant-code-in-Stream.Writable

Conversation

MoonBall commented Jan 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

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

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/_stream_writable.js
}

// reuse the free corkReq.
state.corkedRequestsFree.next = corkReq;

mcollina Jan 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

MoonBall Jan 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina Jan 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/_stream_writable.js

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

mcollina Jan 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

MoonBall Jan 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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:

  1. 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.
  2. if state.finished is false:
    !state.ending && !state.finished equals !state.ending && true.
    !state.ending && !state.finished equals !state.ending.

Comment thread lib/_stream_writable.js

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

mcollina Jan 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

why is this unused?

MoonBall Jan 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina commented Jan 17, 2018

Copy link
Copy Markdown
Member

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

BridgeAR commented Feb 1, 2018

Copy link
Copy Markdown
Member

@mcollina PTAL

mcollina commented Feb 1, 2018

Copy link
Copy Markdown
Member

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

mcollina approved these changes Feb 2, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mcollina commented Feb 2, 2018

Copy link
Copy Markdown
Member

Landed as ce83099 and fca1c55

mcollina closed this Feb 2, 2018
mcollina pushed a commit that referenced this pull request Feb 2, 2018
stream: delete redundant code
ce83099 
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>
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 <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
stream: delete redundant code
8d37cca 
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>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
stream: delete redundant code
524f93f 
`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 pushed a commit that referenced this pull request Feb 21, 2018
stream: delete redundant code
7910c00 
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>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
stream: delete redundant code
4aff6a7 
`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 pushed a commit that referenced this pull request Feb 21, 2018
stream: delete redundant code
560f657 
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>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
stream: delete redundant code
450f5f4 
`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 mentioned this pull request Feb 21, 2018
Merged

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

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

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
stream: delete redundant code
d2187e2 
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
stream: delete redundant code
c83005c 
`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 commented May 23, 2018

Copy link
Copy Markdown
Contributor

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

MylesBorins pushed a commit that referenced this pull request May 23, 2018
stream: delete redundant code
1a3c296 
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>
MylesBorins pushed a commit that referenced this pull request May 23, 2018
stream: delete redundant code
4b2792a 
`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 commented May 23, 2018

Copy link
Copy Markdown
Member

This can be backported.

MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
stream: delete redundant code
eb55e65 
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>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
stream: delete redundant code
1613b0d 
`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 mentioned this pull request Jul 9, 2018
Merged
rvagg pushed a commit that referenced this pull request Aug 16, 2018
stream: delete redundant code
af27768 
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>
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 <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL