Skip to content

stream: Duplex autoDestroy with disabled readable/writable#32139

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

stream: Duplex autoDestroy with disabled readable/writable#32139
ronag wants to merge 4 commits intonodejs:masterfrom
nxtedition:stream-duplex-socket

Conversation

@ronag
Copy link
Copy Markdown
Member

@ronag ronag commented Mar 7, 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.

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
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
Copy link
Copy Markdown
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
Copy Markdown
Member

This requires another @nodejs/tsc review

@ronag
Copy link
Copy Markdown
Member Author

ronag commented Mar 10, 2020

@ronag
Copy link
Copy Markdown
Member Author

ronag commented Mar 10, 2020

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

@lpinca
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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...

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

ronag commented Mar 13, 2020

@jasnell
Copy link
Copy Markdown
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.

@ronag ronag removed the wip Issues and PRs that are still a work in progress. label Mar 14, 2020
@ronag
Copy link
Copy Markdown
Member Author

ronag commented Mar 23, 2020

Copy link
Copy Markdown
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 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
Copy link
Copy Markdown
Member Author

ronag commented Mar 24, 2020

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
Copy link
Copy Markdown
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
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
Copy link
Copy Markdown
Member Author

ronag commented Mar 25, 2020

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

Development

Successfully merging this pull request may close these issues.

8 participants