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

Add AbortSignal.timeout(ms) #951

Closed
domenic opened this issue Feb 24, 2021 · 19 comments · Fixed by #1032
Closed

Add AbortSignal.timeout(ms) #951

domenic opened this issue Feb 24, 2021 · 19 comments · Fixed by #1032
Labels
addition/proposal New features or enhancements topic: aborting AbortController and AbortSignal

Comments

@domenic
Copy link
Member

domenic commented Feb 24, 2021

This has come up in the context of fetch in the past: see whatwg/fetch#951 (comment)

And it has recently come up for streams as well: WICG/serial#122

The proposal is essentially

AbortSignal.timeout = ms => {
  const controller = new AbortController();
  setTimeout(() => controller.abort(), ms);
  return controller.signal;
};

at a high level. Some low-level details:

  • I think we should share the timer task source with setTimeout().
  • I think we should not include the clamping/nesting behavior of setTimeout(), although I'm open to being persuaded otherwise.
  • We should use [EnforceRange] unsigned long long so that you get a quick exception on negative or too-large numbers, instead of clamping negative numbers to zero and taking too-large numbers modulo 2**32.

I'd be happy to write spec text for this if there's implementer interest. Maybe @josepharhar would be interested in implementing this too, similar in spirit to the other recent AbortSignal integration?

@josepharhar
Copy link
Contributor

Consider me interested! I created a tracking bug here.

I think we should share the timer task source with setTimeout().

Does this mean that I should just internally call setTimeout?

I think we should not include the clamping/nesting behavior of setTimeout(), although I'm open to being persuaded otherwise.

What is the clamping/nesting behavior?

We should use [EnforceRange] unsigned long long so that you get a quick exception on negative or too-large numbers, instead of clamping negative numbers to zero and taking too-large numbers modulo 2**32.

I'm guessing this means that I just have to write it like this when I add it to the IDL?

@domenic
Copy link
Member Author

domenic commented Feb 24, 2021

Does this mean that I should just internally call setTimeout?

Nah, I meant it should use the timer task source as opposed to some other task source. In Blink it looks like this means using either kJavascriptTimerImmediate or kJavascriptTimerDelayedLowNesting.

In practice the intention here is that it should have the same "priority" as setTimeout (which is generally relatively low, below important thinks like user interaction).

What is the clamping/nesting behavior?

Per spec steps 11-12, if you nest setTimeout() calls 5 times, then any timeout value <= 4 ms gets clamped to 4 ms.

In implementations, I think things are a bit different, e.g. I believe Chrome always clamps 0 ms to 1 ms, and we also have other minor bugs like https://crbug.com/1108877. See the code for more. I'm basically saying, let's try to bypass that non-interoperable mess, and just always respect the timeout that the web developer passes.

The argument for applying the same clamping to AbortSignal.timeout() would be that the clamping is in place for a sort-of-good reason, and letting people use AbortSignal.timeout(ms).addEventListener("abort", fn) as a non-clamped version of setTimeout(ms, fn) could be bad. But I don't fully buy that. People bypass the clamping already using tricks, so IMO the clamping is more about legacy compatibility at this point.

Note that this per-spec clamping is separate from any user agent policy decisions like throttling or aligning background timers. AbortSignal.timeout() should still be subject to those. In practice I suspect this means just using a given implementation's existing "schedule some JavaScript in the future" functionality.

I'm guessing this means that I just have to write it like this when I add it to the IDL?

Yep, exactly.

@keithamus
Copy link

One thing that's very useful in Golang is the context objects concept of deadlines - and importantly the nesting. One can create a context with a deadline of, say, 10s from now, then a context can be created from that context but the deadline can only be created to have a window as large as the remaining time of the parent deadline.

The reason nested deadlines are useful is because you can dictate nested windows of time at a granular level; for example a server framework might set a deadline for the whole request to be 10s while the logic that makes a db request has a deadline for 2s. Rather than extending the total request time to 12s the child deadline will only be given the total remaining time of the parent, meaning the top most deadlines time window is always honoured.

This could be realised in AbortControllers with timeouts if they exposed the remaining time before a timeout was triggered, perhaps as a getter.

I'm not specifically advocating for this feature as a bandwagon to this issue, but I'd be interested to know if the setTimeout avenue prevents this as a possible future?

@MattiasBuelens
Copy link

@keithamus Interesting! I think we could do that by combining with AbortController.follow() from #920:

async function handleRequest(signal) {
  const dbController = new AbortController();
  dbController.follow(signal);
  dbController.follow(AbortSignal.timeout(2_000));
  await readFromDatabase(dbController.signal);
}

await handleRequest(AbortSignal.timeout(10_000));

The readFromDatabase() call will be aborted after either the parent request's deadline has passed, or the 2-second deadline has passed (whichever happens first).

@annevk annevk added the topic: aborting AbortController and AbortSignal label Feb 25, 2021
@annevk
Copy link
Member

annevk commented Feb 25, 2021

In previous timeout APIs (i.e., XMLHttpRequest), the caller would get to know why something was aborted. I think that's something we should consider here. Whether fetch() should reject with a plain "AbortError" DOMException or something that indicates it happened as the result of a (self-imposed) timeout.

@jakearchibald
Copy link
Collaborator

  • I think we should not include the clamping/nesting behavior of setTimeout(), although I'm open to being persuaded otherwise.

We're trying to reduce the amount of code that executes in the background, and throttling/clamping is part of that. Wouldn't this just become a way to work around it? https://developer.chrome.com/blog/timer-throttling-in-chrome-88/

@domenic
Copy link
Member Author

domenic commented Feb 25, 2021

In previous timeout APIs (i.e., XMLHttpRequest), the caller would get to know why something was aborted. I think that's something we should consider here. Whether fetch() should reject with a plain "AbortError" DOMException or something that indicates it happened as the result of a (self-imposed) timeout.

Hmm, I see the argument. In particular, usually the caller of fetch() wants to ignore aborting caused by AbortSignals since they indicate we don't care. (This leads to API proposals like whatwg/webidl#933.) But in reaction to a timeout, instead you might want to retry, or treat it like a network error.

This means that if we were to do fetch timeouts, we would not do them using signal at all, but instead would need to make them a first-class part of the fetch API. So that brings us back to whatwg/fetch#20.

That definitely puts a damper on my enthusiasm here.

We're trying to reduce the amount of code that executes in the background, and throttling/clamping is part of that. Wouldn't this just become a way to work around it? https://developer.chrome.com/blog/timer-throttling-in-chrome-88/

No. See my response in #951 (comment) where I tried to more clearly differentiate between UA-specific scheduling freedom, and the setTimeout-spec-mandated-but-not-interoperable nesting-based 5 ms clamping.

@Pauan
Copy link

Pauan commented Feb 25, 2021

@domenic Perhaps the AbortError could simply have an additional flag or enum indicating its reason for aborting. Something like error.reason === "timeout". This could be passed in as an argument to abort:

// Triggers an AbortError and assigns "timeout" to the error's `reason` property
controller.abort("timeout");

@annevk
Copy link
Member

annevk commented Feb 26, 2021

Yeah, I was thinking more along the lines of @Pauan's suggestion here. (Perhaps we should even change the class away from DOMException?)

As for the clamping, it seems at least in Gecko that's part of our infrastructure for timers on the web (also applies to https://w3c.github.io/requestidlecallback/#invoke-idle-callback-timeout-algorithm for instance). Bypassing it would end up being more involved.

@annevk
Copy link
Member

annevk commented Mar 3, 2021

@domenic thoughts on the above? I think Fetch and XMLHttpRequest would actually benefit from modeling timeout as an abort reason.

@domenic
Copy link
Member Author

domenic commented Mar 3, 2021

It seems suboptimal to me. I'm fairly convinced that web developers want to treat timeouts differently from aborts (i.e., retry the former, ignore the latter). And we already have a perfectly-good distinction between "TimeoutError" DOMExceptions and "AbortError" DOMExceptions, which XHR already uses. So adding a reason property to DOMException just so that we can reuse "AbortError" DOMException would be confusing, IMO.

@annevk
Copy link
Member

annevk commented Mar 4, 2021

From IRC: I think the big advantage in using abort signals somehow is that we're slowly modifying more and more APIs to accept them. It would be unfortunate to have to make them accept a distinct timeout parameter.

Now, if we did do something like TimeoutSignal it would require updating those APIs to make them account for alternative reasons (and use "TimeoutError"), but seems surmountable (and might also set us up better if we need another one of these in the future).

@Mouvedia
Copy link

Mouvedia commented Jul 9, 2021

Firefox used to coalesce ABORT_ERR and TIMEOUT_ERR. I am in favour of using the right exception/error, i.e. the most descriptive: TimeoutError.

@annevk
Copy link
Member

annevk commented Sep 20, 2021

whatwg/streams#1165 (comment) has an updated take that's worth looking at for those following this issue. The proposal in OP would change to

AbortSignal.timeout = ms => {
  const controller = new AbortController();
  setTimeout(() => controller.abort(new DOMException("TimeoutError"), ms);
  return controller.signal;
};

as I understand it.

@annevk
Copy link
Member

annevk commented Nov 16, 2021

This should now be straightforward to add on top of abort reason (see #1027).

@jasnell
Copy link
Contributor

jasnell commented Nov 16, 2021

Would definitely be +1 on having this as proposed. It would be slightly better if we had ways of refreshing and canceling the timeout once created... e.g for example:

const signal = AbortSignal.timeout(1000);

// wait a few milliseconds somehow... then reset the timout timer.

signal.timeout(1000);

// or cancel the timeout entirely and clear the internal timer
signal.timeout(0);

This would allow me to easily use this for idle-timeouts.

@MattiasBuelens
Copy link

I don't think AbortSignal.prototype.timeout() would be a good API. An AbortSignal should only be able to observe the aborted state, it should not be able to change if or when it becomes aborted. For example, you may want to pass the same AbortSignal to several function calls so you can abort them all at once, but you don't want any single call to cause itself and all other calls to become aborted. Aborting the signal should only be done by the AbortController, so perhaps you'd want AbortController.prototype.timeout() instead. (Although that's still kinda weird...)

Anyway, this goes beyond the scope of the proposed AbortSignal.timeout() method. Let's keep it as a one-shot for now. 🙂

@annevk
Copy link
Member

annevk commented Nov 17, 2021

Feel free to open a new issue to discuss a TimeAbortController or equivalent. (For that it would help to have some list of prior art in other languages, userland adoption, etc.)

@benjamingr
Copy link
Member

Hey, I think we should discuss the error handling story because I think it would be great if there was a way to identify cancellations vs. other operational exceptions.

domenic added a commit that referenced this issue Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: aborting AbortController and AbortSignal
Development

Successfully merging a pull request may close this issue.

10 participants