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.

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

net: don't explicitly set readable/writable #32271

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Mar 14, 2020

net.Socket is slightly breaking stream invariants by having readable/writable going from false to true. Streams assume that readable/writable starts out true and then goes to false through push(null)/end() after which it never goes back to true, e.g. once a stream is writable == false it is assumed it will never become true.

This PR changes 2 things:

Unless explicitly set to false through options:

  • starts as readable/writable true by default.
  • uses push(null)/end() to set readable/writable to false. Note that this would cause the socket to emit the 'end'/'finish' events, which it did not do previously.

In the case it is explicitly set to false through options it is assumed to never become true.

This makes net behave like regular streams and the way quic currently handles the corresponding functionality.

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 net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 14, 2020
@ronag ronag requested a review from lpinca March 14, 2020 21:27
@ronag ronag force-pushed the net-stream-writable-readable branch from d810373 to 0866ef1 Compare March 14, 2020 21:46
@ronag ronag added the wip Issues and PRs that are still a work in progress. label Mar 14, 2020
@ronag ronag removed the request for review from lpinca March 14, 2020 22:04
@ronag

This comment has been minimized.

@ronag ronag closed this Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. 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. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant