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: Duplex autoDestroy with disabled readable/writable #32139

Closed
wants to merge 4 commits into from
Closed

stream: Duplex autoDestroy with disabled readable/writable #32139

wants to merge 4 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Mar 7, 2020 •
edited
Loading

stream.Duplex and net.Socket slightly differs in behavior.

Especially when it comes to the case where one side never
becomes readable or writable. This aligns Duplex with the
behavior of Socket.

The "trick" net.Socket does is to explicitly set writable/readable to false instead of calling end()/push(null) to avoid the 'finish'/'end' events.

This PR extract the streams part of #31806 for easier review.

---- EDIT UPDATED DESCRIPTION ----

Some streams are implemented as Duplex but don't know whether they are unidirectional or not until runtime. In the case that it is determined as unidirectional then one of the sides need to be disabled for e.g. autoDestroy, stream.finished to work properly. However, it doesn't make sense to call end()/push(null) since it never were writable/readable and emitting 'finish'/'close' is weird/confusing.

They way e.g. net.Socket resolves it (and maybe quic and other implementors might want to resolve it? @jasnell) is to explicitly set writable/readable to false once/if it has determined the stream to be unidirectional. This works already, however it would break autoDestroy which (before this PR) waits for both sides to complete, when (in this case) only one is expected complete.

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

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 7, 2020
@ronag
Copy link
Member Author

ronag commented Mar 9, 2020

@vweevers

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 10, 2020
@ronag ronag requested a review from addaleax March 10, 2020 10:06
@mcollina
Copy link
Member

This requires another @nodejs/tsc review

@ronag
Copy link
Member Author

ronag commented Mar 10, 2020

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

@ronag ronag mentioned this pull request Mar 10, 2020
Closed
4 tasks
@ronag
Copy link
Member Author

ronag commented Mar 10, 2020

@jasnell: This might be of interested to you in relation to the quic work.

@ronag ronag mentioned this pull request Mar 11, 2020
Closed
4 tasks
@lpinca
Copy link
Member

lpinca commented Mar 11, 2020

I have no specific objection but I think we should not change the behavior of a generic base class to make it work like one of its derived class.

@ronag
Copy link
Member Author

ronag commented Mar 11, 2020

I have no specific objection but I think we should not change the behavior of a generic base class to make it work like one of its derived class.

Though I would argue that this behavior makes sense... regardless where it's based from?

@Trott
Copy link
Member

Trott commented Mar 12, 2020

This requires another @nodejs/tsc review

@mcollina Do you approve this? I don't usually approve semver-major streams changes unless you approve them first. I have an opinion, but I first defer to the expert...

mcollina
mcollina reviewed Mar 12, 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.

I would need to know more about this change? Can you enrich the description a little bit? It seems very subtle.

I'm especially worried that for a semver-major change we only have so little tests. I would like to ask for a test that covers this behavior.

@ronag
Copy link
Member Author

ronag commented Mar 12, 2020

Yea, I totally missed the tests. Sorry about that. I'll sort this out.

@ronag ronag removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 12, 2020
@ronag
Copy link
Member Author

ronag commented Mar 13, 2020

@mcollina: I have updated the description. Please take a look and let me know if it makes sense. I'll make some proper tests if you find it sensible.

@ronag ronag requested a review from mcollina March 13, 2020 09:33
@ronag
Copy link
Member Author

ronag commented Mar 13, 2020

Please also see the comment here https://github.com/nodejs/node/pull/32158/files#diff-009356850b536cab27b019ba8ad15e72R87-R88

@jasnell
Copy link
Member

jasnell commented Mar 13, 2020

I'm good with this change, although with quic we cannot use autoDestroy as the lifecycle of the object is not tied only to the writable/readable state.

@ronag ronag added the wip Issues and PRs that are still a work in progress. label Mar 14, 2020
@ronag

This comment has been minimized.

Sign in to view
@ronag ronag mentioned this pull request Mar 14, 2020
Closed
4 tasks
@ronag ronag removed the wip Issues and PRs that are still a work in progress. label Mar 14, 2020
@ronag
Copy link
Member Author

ronag commented Mar 23, 2020

@nodejs/streams This PR also fixes the following comment:

https://github.com/nodejs/node/pull/32158/files#diff-009356850b536cab27b019ba8ad15e72R86-R92

mcollina
mcollina requested changes Mar 24, 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.

Can you add a few tests for the change in behavior?

@ronag
stream: align stream.Duplex with net.Socket
ba05b10 
stream.Duplex and net.Socket slightly differs in behavior.

Especially when it comes to the case where one side never
becomes readable or writable. This aligns Duplex with the
behavior of Socket.
@ronag
fixup: more extensive writable check
3e9ba60
@ronag
fixup: tests
fdd369c
@ronag
Copy link
Member Author

ronag commented Mar 24, 2020

rebased and added tests

@ronag ronag changed the title stream: align stream.Duplex with net.Socket stream: stream.Duplex autoDestroy with disabled readable/writable Mar 24, 2020
@ronag ronag changed the title stream: stream.Duplex autoDestroy with disabled readable/writable stream:Duplex autoDestroy with disabled readable/writable Mar 24, 2020
@ronag ronag changed the title stream:Duplex autoDestroy with disabled readable/writable stream: Duplex autoDestroy with disabled readable/writable Mar 24, 2020
mcollina
mcollina approved these changes Mar 24, 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

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

ronag added a commit that referenced this pull request Mar 25, 2020
@ronag
stream: align stream.Duplex with net.Socket
388cef6 
stream.Duplex and net.Socket slightly differs in behavior.

Especially when it comes to the case where one side never
becomes readable or writable. This aligns Duplex with the
behavior of Socket.

PR-URL: #32139
Reviewed-By: Anto Aravinth <anto.aravinth.cse@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 Mar 25, 2020

Landed in 388cef6

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

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@antsmartian antsmartian antsmartian approved these changes

@awibox awibox awibox approved these changes

@lpinca lpinca Awaiting requested review from lpinca

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-major PRs that contain breaking changes and should be released in the next major version. 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.
8 participants
@ronag @nodejs-github-bot @mcollina @lpinca @Trott @jasnell @antsmartian @awibox