Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
stream: abort signal in readable streams #36061
Conversation
|
Review requested: |
|
Idea from Matteo:
Question: what if someone sets it multiple times does it unsubscribe from previous calls? Maybe a static helper for setting an abort signal? |
|
I think it’s better without setter. |
|
@ronag talking to Matteo on FB: I think it's probably better to have a static |
|
Naming suggestions welcome, also ccing @bterlson regarding naming suggestions or opinions here |
|
I've prototyped an alternative approach using a static method here: 6a92d8c - I think the DX is slightly better though still not great? |
6a92d8c
to
52d3dd6
| ); | ||
| // Later, abort the operation closing the stream | ||
| controller.abort(); | ||
| ``` |
jasnell
Nov 20, 2020
Member
An example that shows this used with an async iterator would be good.
An example that shows this used with an async iterator would be good.
| for await (const chunk of read) {} | ||
| })(), /AbortError/); | ||
| setTimeout(() => controller.abort(), 0); | ||
| } |
jasnell
Nov 20, 2020
Member
A test that sets up a pipeline that uses AbortController would be good, as would a test using a single AbortController for two different Readables at the same time.
A test that sets up a pipeline that uses AbortController would be good, as would a test using a single AbortController for two different Readables at the same time.
| })); | ||
| controller.abort(); | ||
| read.on('data', common.mustNotCall()); | ||
| } |
mcollina
Nov 20, 2020
Member
Please add tests for writable as well.
Please add tests for writable as well.
|
@mcollina I've changed the API to work with Readable and Writeable and not just Readable and added a test for that as well. @jasnell added a test for @ronag I'll add the signal as a constructor argument to PTAL :] |
| return !!(obj && typeof obj.pipe === 'function'); | ||
| } | ||
|
|
||
| module.exports = function addAbortSignal(signal, readable) { |
mcollina
Nov 21, 2020
Member
this should not be called readable.
this should not be called readable.
|
|
||
| function isReadable(obj) { | ||
| return !!(obj && typeof obj.pipe === 'function'); | ||
| } |
mcollina
Nov 21, 2020
Member
I don't think this works as you think it would. I think this passes for every descendant of Stream.
I don't think this works as you think it would. I think this passes for every descendant of Stream.
benjamingr
Nov 21, 2020
Author
Member
I think so too and so do the tests - but I just copied isReadable from pipeline
I think so too and so do the tests - but I just copied isReadable from pipeline
Why not in same PR? |
There is already a lot of discussion happening here and I don't feel like making more changes here would make our lives easier basically. |
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
c1fd1fa
to
35a53bb
This comment has been hidden.
This comment has been hidden.
4b21891
to
04cf1c7
Commit Queue failed- Loading data for nodejs/node/pull/36061 ✔ Done loading data for nodejs/node/pull/36061 ----------------------------------- PR info ------------------------------------ Title stream: abort signal in readable streams (#36061) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch benjamingr:abort-signal-stream -> nodejs:master Labels dont-land-on-v10.x, dont-land-on-v12.x, dont-land-on-v14.x, semver-minor, stream Commits 1 - stream: support abort signal Committers 1 - Benjamin Gruenbaum PR-URL: https://github.com/nodejs/node/pull/36061 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36061 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - stream: support abort signal ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-12-07T13:08:26Z: https://ci.nodejs.org/job/node-test-pull-request/34817/ - Querying data for job/node-test-pull-request/34817/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Tue, 10 Nov 2020 08:54:00 GMT ✔ Approvals: 2 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/36061#pullrequestreview-544382792 ✔ - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/36061#pullrequestreview-545601618 -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/406052944 |
Mostly opened for discussion about semantics.
Are these semantics correct? Do they make sense?
This is WIP: I want to test this thoroughly before considering merging + docs need to be updated.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes