Skip to content

timers: allow promisified timeouts/immediates to be canceled#33833

Closed
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:cancel-promisified-timers
Closed

timers: allow promisified timeouts/immediates to be canceled#33833
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:cancel-promisified-timers

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Jun 10, 2020 •
edited
Loading

Using the new experimental AbortController...

const { promisify } = require('util');
const sleep = promisify(setTimeout);

const ac = new AbortController();
const signal = ac.signal;

sleep(1000, undefined, { signal });

ac.abort(); // cancels the setTimeout and rejects the Promise
const { promisify } = require('util');
const immediate = promisify(setImmediate);

const ac = new AbortController();
const signal = ac.signal;

immediate(undefined, { signal });

ac.abort(); // cancels the setImmediate and rejects the Promise

This will necessarily be experimental for as long as AbortController is experimental.

Signed-off-by: James M Snell [email protected]

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 timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jun 10, 2020
@mcollina
Copy link
Member

mcollina commented Jun 10, 2020

I like this. However I think it's time we design a few new apis for setImmediate and setTimeout that are both promisified from the start and abortable.

I do promisify(setTimeout) too many times already.

On a technical side this would have the benefit of avoiding to call promisify to change the AbortController

@jasnell
Copy link
Member Author

jasnell commented Jun 10, 2020

I've been considered exporting the promisified versions of setTimeout and setImmediate as a separate namespace off timers such that we could do...

const {
  setTimeout,
  setImmediate
} = require('timers/promises');

I'm also considering a promisified version of setInterval that returns an async iterator such that...

const {
  setInterval,
} = require('timers/promises');

(async () => {
  for await (const n of setInterval(1000)) {
    console.log('tick...')
  }
})()

We also need to consider how to ref/unref promisified timers/intervals/immediates.

addaleax
addaleax reviewed Jun 11, 2020
View reviewed changes
@jasnell jasnell force-pushed the cancel-promisified-timers branch from 60ee29c to 129a5c2 Compare June 16, 2020 17:01
@jasnell
Copy link
Member Author

jasnell commented Jun 16, 2020

@mcollina ... I will introduce require('timers/promises') in a separate PR

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 16, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 16, 2020

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

@jasnell jasnell added the experimental Issues and PRs related to experimental features. label Jun 16, 2020
@jasnell jasnell requested review from addaleax and devsnek June 16, 2020 19:29
devsnek
devsnek reviewed Jun 16, 2020
View reviewed changes
@jasnell
timers: allow promisified timeouts/immediates to be canceled
b2e0153 
Using the new experimental AbortController...

Signed-off-by: James M Snell <[email protected]>
@jasnell jasnell force-pushed the cancel-promisified-timers branch from 28e3e65 to b2e0153 Compare June 16, 2020 20:12
addaleax
addaleax approved these changes Jun 16, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Nice :)

mcollina
mcollina approved these changes Jun 17, 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

nodejs-github-bot commented Jun 18, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 18, 2020

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

jasnell added a commit that referenced this pull request Jun 18, 2020
@jasnell
timers: allow promisified timeouts/immediates to be canceled
bfbdc84 
Using the new experimental AbortController...

Signed-off-by: James M Snell <[email protected]>

PR-URL: #33833
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jun 18, 2020

Landed in bfbdc84

@jasnell jasnell closed this Jun 18, 2020
@github-actions github-actions bot mentioned this pull request Jun 19, 2020
Open
36 tasks
@jasnell jasnell mentioned this pull request Jun 19, 2020
Closed
4 tasks
This was referenced Jun 20, 2020
Open
Open
Open
@codebytere
Copy link
Member

codebytere commented Jun 22, 2020

Marking dont-land since #33527 is semver-major

@github-actions github-actions bot mentioned this pull request Jun 23, 2020
Open
29 tasks
@addaleax addaleax mentioned this pull request Jun 27, 2020
Closed
3 tasks
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
@jasnell @targos
timers: allow promisified timeouts/immediates to be canceled
18244b6 
Using the new experimental AbortController...

Signed-off-by: James M Snell <[email protected]>

PR-URL: nodejs#33833
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@targos targos mentioned this pull request Apr 24, 2021
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
@jasnell @targos
timers: allow promisified timeouts/immediates to be canceled
59c15fc 
Using the new experimental AbortController...

Signed-off-by: James M Snell <[email protected]>

PR-URL: nodejs#33833
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@jasnell @targos
timers: allow promisified timeouts/immediates to be canceled
c86e131 
Using the new experimental AbortController...

Signed-off-by: James M Snell <[email protected]>

PR-URL: nodejs#33833
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
targos pushed a commit that referenced this pull request Apr 30, 2021
@jasnell @targos
timers: allow promisified timeouts/immediates to be canceled
84b2863 
Using the new experimental AbortController...

Signed-off-by: James M Snell <[email protected]>

PR-URL: #33833
Backport-PR-URL: #38386
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@addaleax addaleax addaleax approved these changes

+1 more reviewer

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

experimental Issues and PRs related to experimental features. semver-minor PRs that contain new features and should be released in the next minor version. 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.

7 participants

@jasnell @mcollina @nodejs-github-bot @codebytere @addaleax @devsnek @targos