Skip to content

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

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

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

Conversation

@jasnell

@jasnelljasnell commented Jun 10, 2020

Copy link
Copy Markdown
Member

Using the new experimental AbortController...

const{ promisify }=require('util');constsleep=promisify(setTimeout);constac=newAbortController();constsignal=ac.signal;sleep(1000,undefined,{ signal });ac.abort();// cancels the setTimeout and rejects the Promise
const{ promisify }=require('util');constimmediate=promisify(setImmediate);constac=newAbortController();constsignal=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 jasnell@gmail.com

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

Copy link
Copy Markdown
Member

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
Copy Markdown
MemberAuthor

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()=>{forawait(constnofsetInterval(1000)){console.log('tick...')}})()

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

Comment threadlib/timers.js Outdated
Comment threadlib/timers.js Outdated
@jasnell
jasnellforce-pushed the cancel-promisified-timers branch from 60ee29c to 129a5c2CompareJune 16, 2020 17:01
@jasnell

Copy link
Copy Markdown
MemberAuthor

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

@jasnelljasnell 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
Copy Markdown
Collaborator

@jasnelljasnell added the experimental Issues and PRs related to experimental features. label Jun 16, 2020
@jasnell
jasnell requested review from addaleax and devsnekJune 16, 2020 19:29
Comment threadlib/timers.js Outdated
Comment threadlib/timers.js Outdated
Using the new experimental AbortController...
Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell
jasnellforce-pushed the cancel-promisified-timers branch from 28e3e65 to b2e0153CompareJune 16, 2020 20:12

@addaleaxaddaleax left a comment

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.

Nice :)

@mcollinamcollina left a comment

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.

lgtm

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

jasnell added a commit that referenced this pull request Jun 18, 2020
Using the new experimental AbortController...
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #33833
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@jasnell

Copy link
Copy Markdown
MemberAuthor

Landed in bfbdc84

@codebytere

Copy link
Copy Markdown
Member

Marking dont-land since #33527 is semver-major

targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
Using the new experimental AbortController...
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#33833
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
targos pushed a commit that referenced this pull request Apr 30, 2021
Using the new experimental AbortController...
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #33833
Backport-PR-URL: #38386
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

experimentalIssues and PRs related to experimental features.semver-minorPRs that contain new features and should be released in the next minor version.timersIssues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

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