stream: Expose DuplexPair API

[awwright]

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 single stream.DuplexPair implementation.

See #29986 for a discussion of this feature.

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

[mscdex]

Why should we be exposing this publicly? Is this a common use case? Is there a reference issue with more information?

[awwright]

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

[awwright]

Would anyone like to speculate why this test creates a duplex pair and then never uses the other side?

[mscdex]

Can we avoid adding this for new APIs? We may as well use a class, which would force new anyway.

[awwright]

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.

[addaleax]

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.

[awwright]

@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.)

[addaleax]

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

[awwright]

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

[addaleax]

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

[awwright]

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?

[addaleax]

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

[mscdex]

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

[awwright]

@mscdex I considered exposing socket1 and socket2, which is what tls uses (the test suite uses clientSide and serverSide). I settled on additionally exposing an array called pair so that you can pick any name you want:

const [ socket1, socket2 ] = new DuplexPair().pair;

This is what TLS now does, in the patch. Do you think that's sufficient?

Some other ideas:

• make the while object iterable, so you can do e.g. const [ a, b ] = DuplexPair();
• socket1 and socket2 (what tls uses)
• upstream and downstream
• inside and outside
• a and b (for brevity when doing const { a: socket1, b: socket2 } = ...)

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 Duplex or PassThrough is, because I'm playing with an idea for a SimplexPair class, which is nothing more than a PassThrough with the addition of SimplexPair#readableSide and SimplexPair#writableSide properties that are only Readable and Writable, respectively.

With this, you can do patterns that maintain encapsulation, like

function generate(){
   const stream = new SimplexPair;
   stream.write("foo");
   return stream.readableSide;
}

[addaleax]

• socket1 and socket2 (what tls uses)

I like this because it matches port1 and port2 from new MessageChannel(). (Alternatively: stream1 and stream2.)

• make the while object iterable, so you can do e.g. const [ a, b ] = DuplexPair();

Fwiw, that’s not exclusive with any naming choices. I like it. 👍

[mscdex]

I'd prefer something even more generic than socket1 and socket2 because this interface doesn't utilize net in any way. Maybe something as simple as side1 and side2?

[awwright]

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 serverSide/clientSide, since a lot of applications will fall into this paradigm.

Do we need the named properties at all, though?

If performance with the iterator function is a concern, you can still do

const { 0: side1, 1: side2 } = new DuplexPair;

How does this sound?

[addaleax]

@nodejs/streams Any further thoughts on this?

[awwright]

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.

[nodejs-github-bot]

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

[mcollina]

The linter is failing, can you check?

[awwright]

@mcollina make test passes for me and I don't have access to three of the failed tests. Is it related to the comment I dropped here?

[mcollina]

@awwright you should run the linter locally with make lint. It will show you the error.

[aduh95]

Can you please rebase and address the linter error?

[awwright]

@aduh95 Is this good?

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[awwright]

I could have sworn all the tests passed when I pushed this, is that blocking the merge?

[mcollina]

I could have sworn all the tests passed when I pushed this, is that blocking the merge?

Yes.

---
duration_ms: 266.324
exitcode: 1
severity: fail
stack: |2-

  node:internal/event_target:1090
    process.nextTick(() => { throw err; });
                             ^
  AssertionError [ERR_ASSERTION]: These builtins are now unexpectedly loaded at run time.
  If this is intentional, add them to `expected.atRunTime`.

  # Note: loading more builtins at run time can lead to startup performance regression.
  - Consider lazy loading builtins that are not used universally.

  --- These could be added to expected.atRunTime ---
  [
    'NativeModule internal/streams/duplexpair'
  ]

      at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/parallel/test-bootstrap-modules.js:242:8)
      at Module._compile (node:internal/modules/cjs/loader:1458:14)
      at Object..js (node:internal/modules/cjs/loader:1542:10)
      at Module.load (node:internal/modules/cjs/loader:1282:32)
      at Function._load (node:internal/modules/cjs/loader:1098:12)
      at TracingChannel.traceSync (node:diagnostics_channel:315:14)
      at wrapModuleLoad (node:internal/modules/cjs/loader:215:24)
      at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:158:5)
      at MessagePort.<anonymous> (node:internal/main/worker_thread:181:26)
      at [nodejs.internal.kHybridDispatch] (node:internal/event_target:816:20) {
    generatedMessage: false,
    code: 'ERR_ASSERTION',
    actual: 7,
    expected: 0,
    operator: 'strictEqual'
  }

  Node.js v23.0.0-pre
...

This is relevant: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/36865/testReport/junit/(root)/parallel/test_bootstrap_modules/