-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(ext/web): Add AbortSignal.timeout()
.
#13687
Conversation
ext/web/03_abort_signal.js
Outdated
// TODO(andreubotella): Don't block the event loop from exiting if there | ||
// aren't any listeners for the abort event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fairly simple with Deno.unrefTimer
& Deno.refTimer
, though they are unstable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure about overriding addEventListener
and removeEventListener
though, with such short timeframe before the release of 1.19.
I don't think this is going to make it for 1.19. There are 2 hours left before I think it would make more sense if we don't actually start the timer until the first listener is registered. We should also cancel the timer once the last listener unregisters. Let's get this landed for 1.20 :-) |
Come to think about it, this would be hard to do right, since per the spec the signal is only aborted in a timer task: const signal = AbortSignal.timeout(10);
console.log(signal.aborted); // false
const start = Date.now();
while (Date.now() - start < 20) {}
console.log(signal.aborted); // must still be false! |
Could be solved with a macrotask callback I think. |
Okay, so let's say that someone checks if the signal has aborted, and the timeout has indeed passed. Do we queue a macrotask now? Because if it has expired a while ago and the event loop hasn't been blocked during that time, the signal should have aborted by the time we're checking. (The spec technically would allow for this, but it's far from ideal.) |
I'm not sure that an implementation that only has an active timer while there are listeners is workable, so I've implemented this with |
@crowlKats @lucacasonato would be great to get this in for 1.20. Please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Closes #13582.