net: make readable/writable start as true #32272

ronag · 2020-03-14T22:51:04Z

net.Socket is slightly breaking stream invariants by having readable/writable going from false to true.

Streams assume that readable/writable either starts out as false and stay that way, or true and then goes to false through push(null)/end() after which it never goes back to true.

This PR changes 2 things:

Unless explicitly set to false through options:

starts as readable/writable true by default.
net.Socket does not manage/set readable/writable, that's internal/private
stream state.
uses push(null)/end() to set readable/writable to false if required after connect. 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 never becomes true.

Currently something like this would fail:

// Uhoh, socket doesn't start as writable until connected? Different from regular streams...
const socket = net.connect(...);
while (socket.writable && chunks.length) {
  socket.write(await chunks.shift());
}
socket.end();

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

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

ronag · 2020-03-14T23:16:55Z

lib/_tls_wrap.js

-    // The readable flag is only needed if pauseOnCreate will be handled.
-    readable: tlsOptions.pauseOnConnect,
-    writable: false
+    manualStart: true


What we actually want here is manualStart since we in the lines below actually do manually start it.

process.nextTick(initRead, this, socket); https://github.com/nodejs/node/pull/32272/files#diff-04c3a6bcf355f2e05e7700c1b253d475L518

lib/_stream_readable.js

lib/_stream_writable.js

nodejs-github-bot · 2020-03-15T00:01:35Z

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

nodejs-github-bot · 2020-03-15T11:47:47Z

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

jasnell · 2020-03-16T15:43:17Z

So a key challenge here is that Streams have never really had a clearly defined semantic around "I'm writable/readable yes, but you can't write or read yet". With both http/2 and quic we've essentially had to hack our way around this by deferring write and read operations until a "ready" event occurs, which is rather messy. What would be ideal here is some kind of proper, performant notion of a ready status that allows writes to be internally buffered prior to the ready state, then passed through like normal after the ready state... that would allow us to avoid the deferral hacks and cork/uncork dance while we're waiting for the underlying socket and communication sessions to be established.

jasnell · 2020-03-16T15:44:28Z

In other words, I'd prefer to have the underlying mechanism in stream.Writable, stream.Readable, and stream.Duplex addressed first then have these specific implementations build on that.

ronag · 2020-03-16T15:55:41Z

In other words, I'd prefer to have the underlying mechanism in stream.Writable, stream.Readable, and stream.Duplex addressed first then have these specific implementations build on that.

I totally agree we need something like that. But I'm unsure whether that should block this? We have a way that it "works" today, shouldn't we make it consistent first? It might take quite a while until we have such a mechanism you suggest.

jasnell · 2020-03-16T18:07:02Z

I totally agree we need something like that. But I'm unsure whether that should block this? We have a way that it "works" today, shouldn't we make it consistent first? It might take quite a while until we have such a mechanism you suggest.

I'm not sure really. I really dislike the hacks we have in place now ;-) .... I won't block this PR waiting for the other mechanism but would definitely like to see work on the other progress.

ronag · 2020-03-16T18:26:16Z

I really dislike the hacks we have in place now ;-)

I agree, my point here though is to try and reduce the hacks... a little bit...

but would definitely like to see work on the other progress.

Definitely something I would like to look into in the future.

ronag · 2020-03-20T14:29:52Z

if this gets merged I think we could eventually deprecate Writable.writable and Readable.readable undocumented but public setters. Only Duplex constructor would need to be able to set them.

nodejs-github-bot · 2020-03-20T14:34:33Z

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

mcollina

LGTM, just 2 nits.
It needs a close look of CITGM

lib/_stream_readable.js

lib/_stream_writable.js

ronag · 2020-03-20T15:12:32Z

@mcollina Added comments, PTAL

ronag · 2020-03-20T15:13:32Z

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

mcollina

lgtm

ronag · 2020-03-20T21:23:36Z

CITGM looks good

ronag · 2020-03-23T16:15:45Z

needs another @nodejs/tsc approval

nodejs-github-bot · 2020-03-23T23:57:11Z

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

ronag · 2020-03-24T08:47:44Z

Landed in eeccd52

`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`. PR-URL: #32272 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

If the socket is writable, a write is attempted and an `ERR_SOCKET_CLOSED` error is emitted because the socket was never connected. Refs: nodejs/node#32272

ronag added 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. labels Mar 14, 2020

ronag requested review from jasnell and addaleax March 14, 2020 22:51

nodejs-github-bot added stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem. labels Mar 14, 2020

ronag force-pushed the net-writable-readbale2 branch 2 times, most recently from 64e26b8 to 33f143b Compare March 14, 2020 23:10

ronag force-pushed the net-writable-readbale2 branch from 33f143b to ba32528 Compare March 14, 2020 23:16

ronag commented Mar 14, 2020

View reviewed changes

lib/_stream_readable.js Show resolved Hide resolved

ronag commented Mar 14, 2020

View reviewed changes

lib/_stream_writable.js Show resolved Hide resolved

ronag requested a review from mcollina March 20, 2020 14:28

mcollina approved these changes Mar 20, 2020

View reviewed changes

lib/_stream_readable.js Show resolved Hide resolved

lib/_stream_writable.js Show resolved Hide resolved

fixup: comments

a8b9f3e

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 20, 2020

mcollina approved these changes Mar 20, 2020

View reviewed changes

jasnell approved these changes Mar 23, 2020

View reviewed changes

ronag closed this Mar 24, 2020

BethGriggs mentioned this pull request Mar 25, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

Ayase-252 mentioned this pull request Apr 15, 2021

Socket.readyState is initially open #38247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: make readable/writable start as true #32272

net: make readable/writable start as true #32272

ronag commented Mar 14, 2020 •

edited

Loading

ronag Mar 14, 2020

ronag Mar 14, 2020 •

edited

Loading

nodejs-github-bot commented Mar 15, 2020

nodejs-github-bot commented Mar 15, 2020

jasnell commented Mar 16, 2020

jasnell commented Mar 16, 2020

ronag commented Mar 16, 2020

jasnell commented Mar 16, 2020

ronag commented Mar 16, 2020

ronag commented Mar 20, 2020 •

edited

Loading

nodejs-github-bot commented Mar 20, 2020

mcollina left a comment

ronag commented Mar 20, 2020

ronag commented Mar 20, 2020

mcollina left a comment

ronag commented Mar 20, 2020

ronag commented Mar 23, 2020

nodejs-github-bot commented Mar 23, 2020

ronag commented Mar 24, 2020

net: make readable/writable start as true #32272

net: make readable/writable start as true #32272

Conversation

ronag commented Mar 14, 2020 • edited Loading

Checklist

ronag Mar 14, 2020

Choose a reason for hiding this comment

ronag Mar 14, 2020 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 15, 2020

nodejs-github-bot commented Mar 15, 2020

jasnell commented Mar 16, 2020

jasnell commented Mar 16, 2020

ronag commented Mar 16, 2020

jasnell commented Mar 16, 2020

ronag commented Mar 16, 2020

ronag commented Mar 20, 2020 • edited Loading

nodejs-github-bot commented Mar 20, 2020

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Mar 20, 2020

ronag commented Mar 20, 2020

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Mar 20, 2020

ronag commented Mar 23, 2020

nodejs-github-bot commented Mar 23, 2020

ronag commented Mar 24, 2020

ronag commented Mar 14, 2020 •

edited

Loading

ronag Mar 14, 2020 •

edited

Loading

ronag commented Mar 20, 2020 •

edited

Loading