Skip to content
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: support passing generator functions into pipeline #31223

Closed
wants to merge 24 commits into from

Conversation

@ronag
Copy link
Member

ronag commented Jan 6, 2020

Add support for generators and functions in pipeline.

This makes it possible to do the following:

await pipeline(
  async function* () {
    yield await read();
  },
  async function*(source) {
    await for (const chunk of source) {
      yield chunk;
    }
  },
  async function(source) {
    await for (const chunk of source) {
      await write(chunk):
    }
  }
);
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
@ronag ronag force-pushed the nxtedition:pipeline-iterable branch 10 times, most recently Jan 6, 2020
@BridgeAR BridgeAR requested review from mcollina and lpinca Jan 6, 2020
@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Jan 6, 2020

I would prefer we did not pass through the stream equivalents for pipeline - I'm not sure if it's feasible. The main reason is performance: streams adds a lot of overhead and a pipeline composed by async iterators can be extremely more performant.

I'm not sure if this is feasible or just a dream.

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Jan 6, 2020

I'm not sure if this is feasible or just a dream.

Might be feasible. I've updated the PR.

@ronag ronag force-pushed the nxtedition:pipeline-iterable branch 13 times, most recently Jan 6, 2020
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Jan 6, 2020

@mcollina: I think you will like this iteration. Now it's possible to do e.g.

let res = '';
pipeline(async function*() {
  await new Promise((resolve) => process.nextTick(resolve));
  yield 'hello';
  yield 'world';
}, async function*(source) {
  for await (const chunk of source) {
    yield chunk.toUpperCase();
  }
}, async function(source) {
  for await (const chunk of source) {
    res += chunk;
  }
}, common.mustCall((err) => {
  assert.strictEqual(err, undefined);
  assert.strictEqual(res, 'HELLOWORLD');
}));

Without passing through the stream equivalents for pipeline.

Still needs some work to ensure edge cases are covered and errors are properly thrown. WIP label please.

@ronag ronag force-pushed the nxtedition:pipeline-iterable branch Jan 6, 2020
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 18, 2020

Landed in 7b78ff0

@Trott Trott closed this Jan 18, 2020
Trott added a commit to Trott/io.js that referenced this pull request Jan 18, 2020
PR-URL: nodejs#31223
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Jan 18, 2020

Notable change?

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Feb 17, 2020

@ronag i tried to backport this to v13.x, but it seems to have broken CI; See https://travis-ci.com/nodejs/node/jobs/288147410

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Feb 17, 2020

@codebytere: Yes, it seems eos is calling the callback earlier in 13.x. I'll make a fix for master which we can backport.

@ronag ronag mentioned this pull request Feb 24, 2020
4 of 4 tasks complete
ronag added a commit to nxtedition/node that referenced this pull request Feb 27, 2020
Backport-PR-URL: nodejs#31975
PR-URL: nodejs#31223
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere added a commit that referenced this pull request Mar 1, 2020
Backport-PR-URL: #31975
PR-URL: #31223
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Mar 1, 2020
codebytere added a commit that referenced this pull request Mar 1, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 4, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 9, 2020

@ronag does this still need a backport?

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Mar 9, 2020

@MylesBorins I've already backported it #31975. Did I miss a label or something?

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 9, 2020

@ronag thanks! when something has been backported we usually apply a different label. backport-open once it has been opened and backported-to when it has landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.