Skip to content

stream: avoid setting writable state#31805

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
Closed
ronag wants to merge 2 commits into from
Closed

stream: avoid setting writable state #31805

ronag wants to merge 2 commits into from

Conversation

@ronag
Copy link
Member

@ronagronag 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-botnodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Feb 15, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2020

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

@ronagronag requested a review from mcollinaFebruary 15, 2020 08:29
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2020

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

@ronag
Copy link
MemberAuthor

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?

@ronagronagforce-pushed the stream-writable-avoid-writable branch from e6a54b6 to 7b70470CompareFebruary 15, 2020 12:14
@ronag
stream: avoid writing to writable
8dd4e55
A remainder from a previous refactoring. Refs: nodejs#31197
@ronagronagforce-pushed the stream-writable-avoid-writable branch from 7b70470 to 8dd4e55CompareFebruary 15, 2020 12:21
@ronagronag requested a review from lpincaFebruary 15, 2020 12:22
mcollina
mcollina requested changes Feb 15, 2020
View reviewed changes
Copy link
Member

@mcollinamcollina 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

@ronagronag requested a review from mcollinaFebruary 16, 2020 09:08
@ronagronagforce-pushed the stream-writable-avoid-writable branch from e9a79e3 to a712f4eCompareFebruary 16, 2020 09:09
@ronagronagforce-pushed the stream-writable-avoid-writable branch from a712f4e to db24c25CompareFebruary 16, 2020 09:09
@ronagronag 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

nodejs-github-bot commented Feb 16, 2020

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

@mcollina
Copy link
Member

mcollina commented Feb 16, 2020

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
MemberAuthor

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

mcollina commented Feb 16, 2020

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
MemberAuthor

ronag commented Feb 16, 2020

can you add a test for that?

db24c25

@mcollina
Copy link
Member

mcollina commented Feb 16, 2020

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
onFinished(stream,state,cb);
}
state.ended=true;
stream.writable=false;
Copy link
MemberAuthor

@ronagronagFeb 16, 2020

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.

@ronagronag changed the title stream: avoid writing to writablestream: avoid setting writable propertyFeb 16, 2020
@ronagronag changed the title stream: avoid setting writable propertystream: avoid setting writable stateFeb 16, 2020
mcollina
mcollina approved these changes Feb 16, 2020
View reviewed changes
Copy link
Member

@mcollinamcollina 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

nodejs-github-bot commented Feb 17, 2020

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

@ronagronag 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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
@ronag
Copy link
MemberAuthor

ronag commented Feb 17, 2020

Landed in 85c6fcd

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

Reviewers

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@lpincalpincalpinca approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.streamIssues 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