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
stream: Expose DuplexPair API #34111
base: main
Are you sure you want to change the base?
Conversation
Why should we be exposing this publicly? Is this a common use case? Is there a reference issue with more information? |
@mscdex I think it's a much more familiar way of creating a Duplex stream for another party, instead of having to use the push/_write/_read/_destroy/_final API. I think there'd be a great benefit if more people moved in the direction of creating a stream using a symmetrical API... see #29986 for a discussion; @addaleax suggested filing a PR over there. |
@@ -14,7 +14,7 @@ const tick = require('../common/tick'); | |||
{ | |||
// This creates a session and schedules a write (for the settings frame). | |||
let client = http2.connect('http://localhost:80', { | |||
createConnection: common.mustCall(() => makeDuplexPair().clientSide) | |||
createConnection: common.mustCall(() => new DuplexPair().clientSide) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would anyone like to speculate why this test creates a duplex pair and then never uses the other side?
lib/_stream_pair.js
Outdated
const clientSide = new DuplexSocket(); | ||
const serverSide = new DuplexSocket(); | ||
function DuplexPair() { | ||
if (!(this instanceof DuplexPair)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid adding this for new APIs? We may as well use a class
, which would force new
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll check it out.
Does Node.js do it this way anywhere else? I was trying to copy how the other stream classes work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use class
for almost everything since class
has been introduced into the language. Unfortunately, we cannot convert this old-style pattern to ES6 classes without a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax Would it be possible to get a deprecation notice for that, then? Or is there some documentation on what causes the breaking change? (My understanding was that class
is just syntactic sugar.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax Would it be possible to get a deprecation notice for that, then?
I don’t think this is something that we’re ever realistically going to be able to change, so there’s no real reason to deprecated it, I assume.
Or is there some documentation on what causes the breaking change? (My understanding was that
class
is just syntactic sugar.)
It’s the fact that classes have to be invoked with new
, and will throw otherwise. That”s not the case in the current code here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax Thanks! I'm working on that now.
Sort of off-topic follow-up, is calling builtins without new
discouraged too then? e.g. const now = Date();
? I've been in discussions with a couple core members on the IRC channel, who've said more-or-less "new is obsolete these days" but I assume that was just a personal opinion around various programming styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awwright Date
is special, because Date()
and new Date()
do different things :)
But yes, for a function that implements a pattern like this (returns an instance of itself even when called without new
), I would recommend using new
because that allows transitioning to classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying this code:
class DuplexPair {
[Symbol.iterator] = Array.prototype[Symbol.iterator];
length = 2;
constructor() {
const clientSide = this[0] = new DuplexSide();
const serverSide = this[1] = new DuplexSide();
this.serverSide = clientSide[kOtherSide] = serverSide;
this.clientSide = serverSide[kOtherSide] = clientSide;
}
}
This passes the test-release
suite, but the acorn dependency can't grok it ("SyntaxError: unexpected token (42:20)" which is at the first closing square bracket). Am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awwright Yeah, we’ve run into trouble with acorn in the past for private properties as well, so I guess this falls into a similar category :/ Fwiw, I think if you do this, you could also use class DuplexPair extends Array
, to automatically get length
and the iterator property…
If we're going to make this public, I think we should be using more generic property names instead of 'client' and 'server' since it's no longer specific to |
@mscdex I considered exposing const [ socket1, socket2 ] = new DuplexPair().pair; This is what TLS now does, in the patch. Do you think that's sufficient? Some other ideas:
I think it'd be OK to expose multiple names so applications can pick one that looks most relevant. To illustrate my thought process, I think it's important to have this be a class like With this, you can do patterns that maintain encapsulation, like function generate(){
const stream = new SimplexPair;
stream.write("foo");
return stream.readableSide;
} |
I like this because it matches
Fwiw, that’s not exclusive with any naming choices. I like it. 👍 |
I'd prefer something even more generic than |
Check my work on the iterable here: function DuplexPair() {
if (!(this instanceof DuplexPair))
return new DuplexPair();
const clientSide = this[0] = new DuplexSide();
const serverSide = this[1] = new DuplexSide();
this.length = 2;
this[Symbol.iterator] = Array.prototype[Symbol.iterator];
clientSide[kOtherSide] = serverSide;
serverSide[kOtherSide] = clientSide;
} afaict, this should behave the same as an Array, and still permit iterating over the other keys in a for..in loop. I like side1 and side2 better than anything else, except I have this hesitation because the numbers don't line up with the array indices, and I'm slightly personally attached to Do we need the named properties at all, though? If performance with the iterator function is a concern, you can still do
How does this sound? |
28d7814
to
892f4e0
Compare
@nodejs/streams Any further thoughts on this? |
@ronag could you take another look? |
Can you rebase on top of main? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think we should expose this. Not blocking but I don't see the use case for it. Is there a heavily used npm package that does this? For me this is more of a slow anti pattern that we shouldn't be encouraging.
Isn't SecurePair basically deprecated?
@ronag ... since you indicated that your objection is non-blocking would you mind clearing your "Request Changes"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think it's a good idea but won't block.
Understood. This requires a rebase anyway so there's still time to discuss and consider. @mcollina, do you have any further thoughts? |
while @ronag disagrees with the change, they've indicated they will not block.
b72fb46
to
dd17b3e
Compare
Rebased; made a few minor changes to appease updated linter standards; and I moved the otherSide reference into a private property, which is a pattern that seems to be used elsewhere Node.js. |
#### `stream.duplexPair([options])` | ||
|
||
<!-- YAML | ||
added: REPLACEME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the linter is upset with this, is this sufficient for now?
11313c0
to
54b64b7
Compare
The linter is failing, can you check? |
@mcollina |
@awwright you should run the linter locally with |
54b64b7
to
83d4b5e
Compare
Co-authored-by: snek <the@snek.dev>
Can you please rebase and address the linter error? |
The "Duplex Pair" pattern is a function that creates two symmetrical Duplex streams, such that what is written on one side will become readable on the other.
This pattern is used by the
tls
module in core, and in a great number of the tests. This PR merges the two implementations together into a singlestream.DuplexPair
implementation.See #29986 for a discussion of this feature.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes