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

lib: initial experimental AbortController implementation #33527

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 23, 2020

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.

const ac = new AbortController();

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-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 23, 2020
@jasnell jasnell force-pushed the abort-controller branch 2 times, most recently from 7544a2d to 29b247c Compare May 23, 2020 03:34
@benjamingr
Copy link
Member

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

Also post it on the summit issue.

'use strict';

// Modeled very closely on the AbortController implementation
// in https://github.com/mysticatea/abort-controller (MIT license)
Copy link
Member

Choose a reason for hiding this comment

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

cc @mysticatea <3

@benjamingr
Copy link
Member

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

@jasnell
Copy link
Member Author

jasnell commented May 23, 2020

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:

const sleep = promisify(setTimeout);
const ac = new AbortController();

sleep(10000, ac);

ac.abort();  // Calls clearTimeout on abort

@benjamingr
Copy link
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
Member Author

jasnell commented May 25, 2020

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.

test/parallel/test-abortcontroller.js Outdated Show resolved Hide resolved
lib/internal/abort_controller.js Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented May 26, 2020

Updated based on the #33556

@jasnell
Copy link
Member Author

jasnell commented May 26, 2020

@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

doc/api/globals.md Show resolved Hide resolved
lib/internal/abort_controller.js Outdated Show resolved Hide resolved
doc/api/globals.md Outdated Show resolved Hide resolved
doc/api/globals.md Outdated Show resolved Hide resolved
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 jasnell force-pushed the abort-controller branch 2 times, most recently from a7b3dd8 to 890a585 Compare May 28, 2020 14:29
@jasnell jasnell marked this pull request as ready for review May 28, 2020 14:31
@jasnell jasnell 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
Member Author

jasnell commented May 28, 2020

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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet