stream: Expose DuplexPair API #34111

awwright · 2020-06-29T00:09:02Z

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 · 2020-06-29T00:41:18Z

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

awwright · 2020-06-29T00:49:19Z

@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 · 2020-06-29T03:40:58Z

test/parallel/test-http2-session-gc-while-write-scheduled.js

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


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

mscdex · 2020-06-29T04:04:54Z

lib/_stream_pair.js

-  const clientSide = new DuplexSocket();
-  const serverSide = new DuplexSocket();
+function DuplexPair() {
+  if (!(this instanceof DuplexPair))


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

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.

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.

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

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

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

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?

@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 · 2020-06-29T04:06:58Z

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 · 2020-06-29T05:29:15Z

@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 · 2020-06-29T16:42:49Z

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 · 2020-06-29T17:12:33Z

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 · 2020-06-29T20:14:47Z

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?

lib/tls.js

lib/_stream_duplexpair.js

addaleax · 2020-07-07T21:27:02Z

@nodejs/streams Any further thoughts on this?

nodejs-github-bot · 2024-07-06T07:51:15Z

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

nodejs-github-bot · 2024-07-06T15:23:48Z

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

nodejs-github-bot · 2024-07-06T16:33:33Z

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

awwright · 2024-07-18T02:41:53Z

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

mcollina · 2024-07-18T13:48:00Z

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/

nodejs-github-bot · 2024-07-23T06:48:50Z

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

nodejs-github-bot · 2024-07-25T13:54:52Z

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

nodejs-github-bot · 2024-07-26T07:20:42Z

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

mcollina

lgtm

github-actions · 2024-07-26T08:02:21Z

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @mcollina.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

nodejs-github-bot · 2024-07-26T08:09:24Z

Landed in b32732b

PR-URL: #34111 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: TODO

Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123

PR-URL: #34111 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 29, 2020

awwright force-pushed the makeDuplexPair branch from b6946fd to 207c12d Compare June 29, 2020 00:09

awwright changed the title ~~Expose DuplexPair API~~ stream: Expose DuplexPair API Jun 29, 2020

awwright force-pushed the makeDuplexPair branch from 207c12d to 3e98449 Compare June 29, 2020 00:43

awwright commented Jun 29, 2020

View reviewed changes

mscdex reviewed Jun 29, 2020

View reviewed changes

awwright force-pushed the makeDuplexPair branch from 3e98449 to d0feea3 Compare June 29, 2020 20:07

awwright force-pushed the makeDuplexPair branch 6 times, most recently from 28d7814 to 892f4e0 Compare July 1, 2020 03:19

awwright requested review from mscdex and addaleax July 1, 2020 19:27

awwright commented Jul 1, 2020

View reviewed changes

lib/tls.js Outdated Show resolved Hide resolved

addaleax reviewed Jul 1, 2020

View reviewed changes

lib/_stream_duplexpair.js Outdated Show resolved Hide resolved

lib/_stream_duplexpair.js Outdated Show resolved Hide resolved

awwright force-pushed the makeDuplexPair branch from ee5cf57 to 0c116a9 Compare July 1, 2020 20:12

awwright closed this Jul 1, 2020

awwright force-pushed the makeDuplexPair branch from 0c116a9 to 6213fce Compare July 1, 2020 20:41

awwright reopened this Jul 1, 2020

addaleax approved these changes Jul 1, 2020

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2024

stream: expose DuplexPair API

594f2cd

awwright force-pushed the makeDuplexPair branch from 26f9013 to 594f2cd Compare July 22, 2024 20:01

mcollina added semver-minor PRs that contain new features and should be released in the next minor version. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 23, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2024

mcollina approved these changes Jul 26, 2024

View reviewed changes

mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Jul 26, 2024

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 26, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 26, 2024

nodejs-github-bot merged commit b32732b into nodejs:main Jul 26, 2024
62 checks passed

targos pushed a commit that referenced this pull request Jul 28, 2024

stream: expose DuplexPair API

f5633b1

PR-URL: #34111 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

RafaelGSS mentioned this pull request Jul 30, 2024

v22.6.0 proposal #54123

Merged

RafaelGSS pushed a commit that referenced this pull request Aug 5, 2024

stream: expose DuplexPair API

15816bd

PR-URL: #34111 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Jul	AUG	Sep
	08
2023	2024	2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: Expose DuplexPair API #34111

stream: Expose DuplexPair API #34111

awwright commented Jun 29, 2020 •

edited

Loading

mscdex commented Jun 29, 2020

awwright commented Jun 29, 2020 •

edited

Loading

awwright Jun 29, 2020

mscdex Jun 29, 2020

awwright Jun 29, 2020 •

edited

Loading

addaleax Jun 29, 2020

awwright Jun 29, 2020

addaleax Jun 29, 2020

awwright Jun 29, 2020 •

edited

Loading

addaleax Jun 29, 2020

awwright Jun 29, 2020

addaleax Jun 29, 2020

mscdex commented Jun 29, 2020

awwright commented Jun 29, 2020

addaleax commented Jun 29, 2020

mscdex commented Jun 29, 2020

awwright commented Jun 29, 2020 •

edited

Loading

addaleax commented Jul 7, 2020

nodejs-github-bot commented Jul 6, 2024

nodejs-github-bot commented Jul 6, 2024

nodejs-github-bot commented Jul 6, 2024

awwright commented Jul 18, 2024

mcollina commented Jul 18, 2024

nodejs-github-bot commented Jul 23, 2024

nodejs-github-bot commented Jul 25, 2024

nodejs-github-bot commented Jul 26, 2024

mcollina left a comment

github-actions bot commented Jul 26, 2024

nodejs-github-bot commented Jul 26, 2024

stream: Expose DuplexPair API #34111

stream: Expose DuplexPair API #34111

Conversation

awwright commented Jun 29, 2020 • edited Loading

Checklist

mscdex commented Jun 29, 2020

awwright commented Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwright Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwright Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jun 29, 2020

awwright commented Jun 29, 2020

addaleax commented Jun 29, 2020

mscdex commented Jun 29, 2020

awwright commented Jun 29, 2020 • edited Loading

addaleax commented Jul 7, 2020

nodejs-github-bot commented Jul 6, 2024

nodejs-github-bot commented Jul 6, 2024

nodejs-github-bot commented Jul 6, 2024

awwright commented Jul 18, 2024

mcollina commented Jul 18, 2024

nodejs-github-bot commented Jul 23, 2024

nodejs-github-bot commented Jul 25, 2024

nodejs-github-bot commented Jul 26, 2024

mcollina left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 26, 2024

nodejs-github-bot commented Jul 26, 2024

awwright commented Jun 29, 2020 •

edited

Loading

awwright commented Jun 29, 2020 •

edited

Loading

awwright Jun 29, 2020 •

edited

Loading

awwright Jun 29, 2020 •

edited

Loading

awwright commented Jun 29, 2020 •

edited

Loading