Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream: avoid setting writable state #31805

Closed
wants to merge 2 commits into from
Closed

stream: avoid setting writable state #31805

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Feb 15, 2020 •
edited
Loading

A remainder from a previous refactoring.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Feb 15, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag requested a review from mcollina February 15, 2020 08:29
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Feb 15, 2020

@Trott: Problems getting CI to pass:

tap2junit: error: argument --input/-i: can't open 'test.tap': [Errno 2] No such file or directory: 'test.tap'

Any advice?

@ronag ronag force-pushed the stream-writable-avoid-writable branch from e6a54b6 to 7b70470 Compare February 15, 2020 12:14
@ronag
stream: avoid writing to writable
8dd4e55 
A remainder from a previous refactoring.

Refs: nodejs#31197
@ronag ronag force-pushed the stream-writable-avoid-writable branch from 7b70470 to 8dd4e55 Compare February 15, 2020 12:21
@ronag ronag requested a review from lpinca February 15, 2020 12:22
mcollina
mcollina requested changes Feb 15, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

This needs unit tests

@ronag ronag requested a review from mcollina February 16, 2020 09:08
@ronag ronag force-pushed the stream-writable-avoid-writable branch from e9a79e3 to a712f4e Compare February 16, 2020 09:09
@ronag ronag force-pushed the stream-writable-avoid-writable branch from a712f4e to db24c25 Compare February 16, 2020 09:09
@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@mcollina
Copy link
Member

I don't understand the reasoning behind this change, or what effect it would have on our APIs. Can you please elaborate? Note that the above link is a 404

@ronag
Copy link
Member Author

ronag commented Feb 16, 2020 •
edited
Loading

I don't understand the reasoning behind this change, or what effect it would have on our APIs. Can you please elaborate? Note that the above link is a 404

It doesn't change the API at all. We did a refactoring in #31197 to make Writable.writable computed. But we left a legacy mode to still allow writing to the property.

This PR doesn't change any API behavior. However, it fixes an unnecessary write to writable which would unnecessarily add a property on the state object. The observable state should remain the same. This should have been removed in #31197 but I missed it.

@mcollina
Copy link
Member

it fixes an unnecessary write to writable which would unnecessarily add a property on the state object

can you add a test for that?

@ronag
Copy link
Member Author

ronag commented Feb 16, 2020

can you add a test for that?

db24c25

@mcollina
Copy link
Member

I’m lost. You said it would save a call to write() and I cannot see how.

ronag
ronag commented Feb 16, 2020
View reviewed changes
lib/_stream_writable.js
@@ -687,7 +687,6 @@ function endWritable(stream, state, cb) {
onFinished(stream, state, cb);
}
state.ended = true;
stream.writable = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

@mcollina: The point is to remove this. It’s unnecessary (it should already be false through the computed getter) and causes the ‘writable’ property to be unnecessarily added on the state object.

@ronag ronag changed the title stream: avoid writing to writable stream: avoid setting writable property Feb 16, 2020
@ronag ronag changed the title stream: avoid setting writable property stream: avoid setting writable state Feb 16, 2020
mcollina
mcollina approved these changes Feb 16, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag mentioned this pull request Feb 17, 2020
Closed
4 tasks
ronag added a commit that referenced this pull request Feb 17, 2020
@ronag
stream: avoid writing to writable
85c6fcd 
A remainder from a previous refactoring.

Refs: #31197

PR-URL: #31805
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ronag
Copy link
Member Author

ronag commented Feb 17, 2020

Landed in 85c6fcd

@ronag ronag closed this Feb 17, 2020
@BethGriggs BethGriggs mentioned this pull request Apr 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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
@ronag @nodejs-github-bot @mcollina @jasnell @lpinca