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: abort signal in readable streams #36061

Closed
wants to merge 1 commit into from

Conversation

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Nov 10, 2020

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@benjamingr benjamingr added the stream label Nov 10, 2020
@benjamingr benjamingr requested review from mcollina and ronag Nov 10, 2020
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Nov 10, 2020

Review requested:

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 10, 2020

Idea from Matteo:

  • Expose an .abortSignal property on stream.Readable with:
    • A getter that returns the signal if present or null otherwise.
    • A setter that sets the abort signal on the stream.

Question: what if someone sets it multiple times does it unsubscribe from previous calls?

Maybe a static helper for setting an abort signal?

@ronag
Copy link
Member

@ronag ronag commented Nov 10, 2020

I think it’s better without setter.

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 10, 2020

@ronag talking to Matteo on FB: I think it's probably better to have a static shedEventTarget (name pending) method and probably not expose a getter/setter to avoid confusion about "what happens if we call a setter on multiple signals" and to avoid versioning issues.

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 10, 2020

Naming suggestions welcome, also ccing @bterlson regarding naming suggestions or opinions here

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 10, 2020

I've prototyped an alternative approach using a static method here: 6a92d8c - I think the DX is slightly better though still not great?

lib/internal/streams/add-abort-signal.js Outdated Show resolved Hide resolved
@benjamingr benjamingr marked this pull request as ready for review Nov 20, 2020
@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 20, 2020

@mcollina @ronag please take a look I've updated this PR based on the meeting consensus namely - streams are destroyed with an error

);
// Later, abort the operation closing the stream
controller.abort();
```

This comment has been minimized.

@jasnell

jasnell Nov 20, 2020
Member

An example that shows this used with an async iterator would be good.

for await (const chunk of read) {}
})(), /AbortError/);
setTimeout(() => controller.abort(), 0);
}

This comment has been minimized.

@jasnell

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.

lib/internal/streams/add-abort-signal.js Outdated Show resolved Hide resolved
doc/api/stream.md Show resolved Hide resolved
}));
controller.abort();
read.on('data', common.mustNotCall());
}

This comment has been minimized.

@mcollina

mcollina Nov 20, 2020
Member

Please add tests for writable as well.

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 21, 2020

@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 pipeline and an example with async iterators in the README.

@ronag I'll add the signal as a constructor argument to Readable and Writeable like in the original iteration of this PR in a follow up PR.

PTAL :]

lib/internal/streams/add-abort-signal.js Outdated
return !!(obj && typeof obj.pipe === 'function');
}

module.exports = function addAbortSignal(signal, readable) {

This comment has been minimized.

@mcollina

mcollina Nov 21, 2020
Member

this should not be called readable.

lib/internal/streams/add-abort-signal.js Show resolved Hide resolved

function isReadable(obj) {
return !!(obj && typeof obj.pipe === 'function');
}

This comment has been minimized.

@mcollina

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.

This comment has been minimized.

@benjamingr

benjamingr Nov 21, 2020
Author Member

I think so too and so do the tests - but I just copied isReadable from pipeline

lib/internal/streams/add-abort-signal.js Outdated Show resolved Hide resolved
lib/internal/streams/add-abort-signal.js Outdated Show resolved Hide resolved
lib/internal/streams/readable.js Outdated Show resolved Hide resolved
lib/internal/streams/add-abort-signal.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member

@ronag ronag commented Nov 21, 2020

@ronag I'll add the signal as a constructor argument to Readable and Writeable like in the original iteration of this PR in a follow up PR.

Why not in same PR?

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 21, 2020

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.

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 21, 2020

@mcollina @ronag addressed :]

@github-actions
Copy link

@github-actions github-actions bot commented Dec 7, 2020

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/.ncu
https://github.com/nodejs/node/actions/runs/406052944
@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Dec 7, 2020

@mcollina can you LGTM after the last rebase so I can land with the commit queue? I'm hesitant to land with ncu locally if I can help it :]

Copy link
Member

@mcollina mcollina left a comment

lgtm

@github-actions
Copy link

@github-actions github-actions bot commented Dec 7, 2020

Landed in 5122456...5bd1eec

@github-actions github-actions bot closed this Dec 7, 2020
nodejs-github-bot added a commit that referenced this pull request Dec 7, 2020
PR-URL: #36061
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Dec 7, 2020

Next up, the constructor arg version.

@benjamingr benjamingr deleted the benjamingr:abort-signal-stream branch Dec 7, 2020
danielleadams added a commit that referenced this pull request Dec 7, 2020
PR-URL: #36061
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
danielleadams added a commit that referenced this pull request Dec 7, 2020
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
cjihrig added a commit to cjihrig/node that referenced this pull request Dec 8, 2020
PR-URL: nodejs#36061
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
danielleadams added a commit that referenced this pull request Dec 9, 2020
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
danielleadams added a commit that referenced this pull request Dec 9, 2020
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
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

5 participants