Skip to content

lib: initial experimental AbortController implementation - #33527

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:abort-controller
Closed

lib: initial experimental AbortController implementation#33527
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:abort-controller

Conversation

@jasnell

@jasnelljasnell commented May 23, 2020

Copy link
Copy Markdown
Member

AbortController impl based very closely on:
https://github.com/mysticatea/abort-controller

Marked experimental.
Global (writable, configurable)
Not currently used by any of the existing promise apis.
AbortSignal extends from EventEmitter instead of EventTarget.

constac=newAbortController();ac.onabort=()=>{};// Or ...ac.on('abort',()=>{});ac.abort();console.log(ac.aborted);
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

@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 23, 2020
@jasnell
jasnellforce-pushed the abort-controller branch 2 times, most recently from 7544a2d to 29b247cCompareMay 23, 2020 03:34
@benjamingr

Copy link
Copy Markdown
Member

That's awesome, I'm going to check this out and play with it asap.

Also post it on the summit issue.

Comment threadlib/internal/abort_controller.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment threadlib/internal/abort_controller.js Outdated
Comment thread.eslintrc.js Outdated
@benjamingr

Copy link
Copy Markdown
Member

Other than test and review this is there anything I can do to help?

@jasnell

Copy link
Copy Markdown
MemberAuthor

Other than test and review this is there anything I can do to help?

One of the next steps is to identify a list of the existing Promise APIs in core that could by updated to make use of this. Not all of them will make sense because not all of them are abortable (e.g. libuv only allows us to cancel file system requests if they have not already been started, for instance). So while we may be able to use AbortController for fs.promises.readFile() (which performs a sequence of discreet read operations) it won't for fs.promises.read() (which performs a single discreet read operation). Once we have a list of APIs for which use of AbortController makes sense, then we need to figure out the best way to incorporate it. Each will have their own set of considerations to address.

Additionally, we should make it possible for custom util.promisify implementations to support AbortController. For instance, I'm playing around with the promisification of setTimeout such that it allows:

constsleep=promisify(setTimeout);constac=newAbortController();sleep(10000,ac);ac.abort();// Calls clearTimeout on abort

@benjamingr

Copy link
Copy Markdown
Member

@jasnell I've split those tasks off into an issue here: #33528 - please feel free to edit that issue as you see fit.

I will start working on these one by one (on weekends mostly) and add tasks as I find them. Feel free to edit it.

I want to be clear that I want to be helpful here so since you took the initiative if at any point any of the things I'm doing are frustrating just tell me and I'll stop and do whatever is helpful to the effort instead. These sorts of ongoing efforts tend to become frustrating in Node.js at times, I want to be sure that I'm not doing any toe-stepping if I can help it.

@jasnell

Copy link
Copy Markdown
MemberAuthor

Initial implementation of EventTarget in #33556. Assuming that one goes through, this PR will be modified such that AbortController.prototype.signal is an instance of EventTarget rather than EventEmitter.

Comment threadtest/parallel/test-abortcontroller.js Outdated
Comment threadlib/internal/abort_controller.js Outdated
@jasnell
jasnellforce-pushed the abort-controller branch 2 times, most recently from 02953f1 to 7290192CompareMay 26, 2020 22:46
@jasnell

Copy link
Copy Markdown
MemberAuthor

Updated based on the #33556

@jasnell

Copy link
Copy Markdown
MemberAuthor

@benjamingr ... quick note... this is currently extending from NodeEventTarget ... in this, however, the remove listener functions are not used and the only ones we definitely need here are the on/once/addListener in order to achieve the use cases we need for AbortSignal

Comment threaddoc/api/globals.md Outdated
Comment threadlib/internal/abort_controller.js Outdated
Comment threaddoc/api/globals.md Outdated
Comment threaddoc/api/globals.md Outdated
jasnell added a commit that referenced this pull request May 28, 2020
See documentation changes for details
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #33556
Refs: #33527
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
@jasnell
jasnellforce-pushed the abort-controller branch 2 times, most recently from a7b3dd8 to 890a585CompareMay 28, 2020 14:29
@jasnell
jasnell marked this pull request as ready for review May 28, 2020 14:31
@jasnelljasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label May 28, 2020
@jasnell

Copy link
Copy Markdown
MemberAuthor

Marking this ready for review. This does create a new global so technically may be semver-major but I have tested it with existing ecosystem polyfills and haven't seen any breakage at all. Unless there are objections from the @nodejs/tsc, I'd like to handle this as a semver-minor.

It is still experimental and an experimental warning will be emitted the first time an AbortController is created. It is not behind a flag.

codebytere added a commit to electron/electron that referenced this pull request May 18, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / srcIssues and PRs related to general changes in the lib or src directory.semver-majorPRs that contain breaking changes and should be released in the next major version.tsc-agendaIssues and PRs to discuss during the meetings of the TSC.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@jasnell@benjamingr@nodejs-github-bot@mcollina@richardlau@targos@mhdawson@zcbenz@addaleax@ptomato@devsnek@BridgeAR@lundibundi@felixfbecker@Ethan-Arrowood