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.

Sign up for GitHub

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

Jump to bottom

timers: reject with AbortError on cancellation #36317

Closed
wants to merge 1 commit into from
Closed

timers: reject with AbortError on cancellation #36317

wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

Following the consensus from the AbortSignal behaviour meeting (at #36084 )

This replaces the DOMException timers/promises reject with with a Node AbortError (like HTTP/HTTP2 and streams)

From a user point of view it has the same .name and .code but is easier to shim, even the docs and tests don't need to change :]

cc @mcollina @jasnell @ronag

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 timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. promises Issues and PRs related to ECMAScript promises. labels Nov 29, 2020
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34618/

mcollina
mcollina approved these changes Nov 29, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34624/

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 1, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 1, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/36317
✔  Done loading data for nodejs/node/pull/36317
----------------------------------- PR info ------------------------------------
Title      timers: reject with AbortError on cancellation (#36317)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     benjamingr:timers-promises-dom-exception -> nodejs:master
Labels     promises, timers
Commits    1
 - timers: reject with AbortError on cancellation
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/36317
Reviewed-By: Michaël Zasso 
Reviewed-By: James M Snell 
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36317
Reviewed-By: Michaël Zasso 
Reviewed-By: James M Snell 
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-11-30T09:05:21Z: https://ci.nodejs.org/job/node-test-pull-request/34624/
- Querying data for job/node-test-pull-request/34624/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sun, 29 Nov 2020 18:04:46 GMT
   ✔  Approvals: 4
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/36317#pullrequestreview-540503231
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/36317#pullrequestreview-540503892
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/36317#pullrequestreview-540505016
   ✔  - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/36317#pullrequestreview-540511194
   ✖  This PR needs to wait 28 more minutes to land
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/394367476

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 1, 2020
@benjamingr benjamingr added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 1, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 1, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2020

Landed in 780fcb4...1ed72f6

@github-actions github-actions bot closed this Dec 1, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 1, 2020
@benjamingr @nodejs-github-bot
timers: reject with AbortError on cancellation
1ed72f6 
PR-URL: #36317
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
@benjamingr @danielleadams
timers: reject with AbortError on cancellation
29b5236 
PR-URL: #36317
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
Merged
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
@benjamingr @cjihrig
timers: reject with AbortError on cancellation
5f5d28b 
PR-URL: nodejs#36317
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
targos added a commit that referenced this pull request May 16, 2021
@targos
timers: reject with AbortError on cancellation
b41bd4f 
PR-URL: #36317
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@targos targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@benjamingr @targos
timers: reject with AbortError on cancellation
6799738 
PR-URL: #36317
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronag ronag ronag approved these changes

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
promises Issues and PRs related to ECMAScript promises. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@benjamingr @nodejs-github-bot @mcollina @jasnell @targos @ronag