X Tutup
The Wayback Machine - https://web.archive.org/web/20221127013732/https://github.com/nodejs/node/pull/33527
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
@benjamingr
Copy link
Member

benjamingr commented May 23, 2020

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

@benjamingr benjamingr May 23, 2020

Choose a reason for hiding this comment

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

cc @mysticatea <3

lib/internal/abort_controller.js Outdated Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
@benjamingr
Copy link
Member

benjamingr commented May 23, 2020

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

benjamingr commented May 23, 2020

@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
@jasnell jasnell marked this pull request as ready for review May 28, 2020
@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.

@benjamingr
Copy link
Member

benjamingr commented Oct 30, 2020

@zcbenz

I assume Node's APIs would not be able to take DOM's as well.

Why not? We do not perform the same sort of branding checks the DOM does. We just check that it has a property called "aborted" on it at the moment.

I cannot speak about the other way around though - I would assume node's AbortSignals won't work in DOM APIs.

The TypeScript definitions of Node's AbortSignal must be exactly the same with DOM's one,

I am not sure who maintains the node type definitions - but if the API is different please open an issue and we will fix it. A goal is that developers should have an easier time cancelling things in a universal way and API deltas run counter to that.


But if Node's promise APIs are able to take arbitrary implementation of AbortSignal, then we would be able to replace Node's AbortController with DOM's one completely, and all of the above issues would be solved.

This sounds like a reasonable approach to me.

@targos targos added the backport-open-v14.x Indicate that the PR has an open backport. label Apr 24, 2021
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
AbortController impl based very closely on:
 https://github.com/mysticatea/abort-controller

Marked experimental.
Not currently used by any of the existing promise apis.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#33527
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
AbortController impl based very closely on:
 https://github.com/mysticatea/abort-controller

Marked experimental.
Not currently used by any of the existing promise apis.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#33527
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
AbortController impl based very closely on:
 https://github.com/mysticatea/abort-controller

Marked experimental.
Not currently used by any of the existing promise apis.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#33527
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Apr 30, 2021
AbortController impl based very closely on:
 https://github.com/mysticatea/abort-controller

Marked experimental.
Not currently used by any of the existing promise apis.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #33527
Backport-PR-URL: #38386
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos added backported-to-v14.x PRs backported to the v14.x-staging branch. and removed backport-open-v14.x Indicate that the PR has an open backport. labels Apr 30, 2021
@danielleadams danielleadams mentioned this pull request May 3, 2021
codebytere added a commit to electron/electron that referenced this pull request May 14, 2021
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
backported-to-v14.x PRs backported to the v14.x-staging branch. 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

X Tutup