net: cleans up 4 instances of internal stream access #2570
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
net uses decodeStrings=false, but previously this was set directly on the writableState, which is a big no-no even for core. Instead, create a copy of the options and set decodeStrings as we give it to the Duplex constructor.
Previously, net reached into the readableState to pause the stream, which is a big no-no. This commit changes it to use ReadableStream#pause instead, which is cleaner. RS#pause emits a 'pause' event, which would have been a change in behaviour, but because servers only emit the 'connection' event after the Socket is created - and the 'pause' event is thrown synchronously, there are no sockets to listen to for the 'pause' event so there is no change in behaviour. Introduced in c2b4f48.
This was unnecessary because - as the comment points out - on EOF, the readable stream will set ended = true for us, even if the 'end' event has yet to be emitted.
This should be unnecessary, as Readable emits the 'end' event on a next tick, while '_socketEnd' is only triggered synchronously. Therefore endEmitted should always be false when the runtime enters that function. This commit collapses that if / else statement, which was redundant, and adds an assert statement *just in case* to make sure that there was no 'end' event emitted. It also adds a few comments to make the onSocketEnd more clearer for readers. The code originally came from the net rewrite to streams 2, which leads me to believe it was added because it was *thought* it would be followed, rather than it *would* be followed. See 8a3befa.
|
cc @nodejs/streams |
|
First three commits LGTM, a little less sure about the last and generally want more eyes on this streams stuff. CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/201/ |
|
@Fishrock123 thanks, CI is happy minus #2464. I'm mostly confident about the last commit for regular use cases, but I can remove it if we're not 100% sure. I don't want to risk breaking things in this, as it isn't any more than a cleanup PR. |
|
I'll probably drop the last one since I'm not 100% about it. Perhaps @chrisdickinson wants to also take a look? |
|
It has been a long time and none of these are really more than code cleanup... I'll close this for now, but if someone wants to pick it up, be my guest. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request contains 4 commits, each which clean up an individual instance of stream
_writableStateor_readableStateaccess. Each commit contains a nice explanation on how each change works and why it should work the same.If node can't use the public stream APIs, why should anyone else?
Ref to: #445