Skip to content

stream: Duplex autoDestroy with disabled readable/writable - #32139

Closed
ronag wants to merge 4 commits into
nodejs:masterfrom
nxtedition:stream-duplex-socket
Closed

ronag wants to merge 4 commits into
nodejs:masterfrom
nxtedition:stream-duplex-socket

Conversation

@ronag

@ronag ronag commented Mar 7, 2020 •

Copy link
Copy Markdown
Member

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 Node.js streams. label Mar 7, 2020
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronag
ronag removed request for BridgeAR and antsmartian March 7, 2020 22:45
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@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

ronag commented Mar 9, 2020

Copy link
Copy Markdown
Member Author

@vweevers

@ronag ronag added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 10, 2020
@ronag
ronag requested a review from addaleax March 10, 2020 10:06
@mcollina

Copy link
Copy Markdown
Member

This requires another @nodejs/tsc review

@ronag

ronag commented Mar 10, 2020

Copy link
Copy Markdown
Member Author

@ronag

ronag commented Mar 10, 2020

Copy link
Copy Markdown
Member Author

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

@lpinca

lpinca commented Mar 11, 2020

Copy link
Copy Markdown
Member

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

ronag commented Mar 11, 2020

Copy link
Copy Markdown
Member Author

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

Trott commented Mar 12, 2020

Copy link
Copy Markdown
Member

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

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

ronag commented Mar 12, 2020

Copy link
Copy Markdown
Member Author

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

@ronag ronag removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 12, 2020
@ronag

ronag commented Mar 13, 2020

Copy link
Copy Markdown
Member Author

@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

ronag commented Mar 13, 2020

Copy link
Copy Markdown
Member Author

@jasnell

jasnell commented Mar 13, 2020

Copy link
Copy Markdown
Member

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.

@ronag ronag mentioned this pull request Mar 14, 2020
3 of 4 tasks
@ronag ronag removed the wip Issues and PRs that are still a work in progress. label Mar 14, 2020
@ronag

ronag commented Mar 23, 2020

Copy link
Copy Markdown
Member Author

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

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

@mcollina 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.

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

ronag added 3 commits March 24, 2020 14:03
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

ronag commented Mar 24, 2020

Copy link
Copy Markdown
Member Author

rebased and added tests

@ronag
ronag force-pushed the stream-duplex-socket branch from 2696a6e to fdd369c Compare March 24, 2020 13:03
@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 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.

lgtm

@ronag ronag added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 24, 2020
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

ronag added a commit that referenced this pull request Mar 25, 2020
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ronag

ronag commented Mar 25, 2020

Copy link
Copy Markdown
Member Author

Landed in 388cef6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to Node.js streams.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants