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

AbortSignal.timeout() #711

Closed
1 task done
shaseley opened this issue Jan 28, 2022 · 10 comments
Closed
1 task done

AbortSignal.timeout() #711

shaseley opened this issue Jan 28, 2022 · 10 comments
Assignees
Labels
Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Resolution: timed out The TAG has requesed additional information but has not received it Review type: later review Review type: small delta Topic: DOM About or relating to the Document Object Model Topic: scripting ECMA, Web Assembly bindings, etc. Venue: WHATWG

Comments

@shaseley
Copy link

Braw mornin' TAG!

I'm requesting a TAG review of AbortSignal.timeout(milliseconds).

This introduces a new static method, AbortSignal.timeout(milliseconds), which returns a newly-created AbortSignal that, after milliseconds milliseconds, becomes automatically aborted. The AbortSignal's reason property will be a "TimeoutError" DOMException. The main motivating use case for this is helping web developers easily time out async operations, such as fetch().

Further details:

  • I have reviewed the TAG's Web Platform Design Principles
  • Relevant time constraints or deadlines: No deadline, but the spec PR has been approved and has support from multiple implementers, so I'd expect the first implementation and PR will both land soon.
  • The group where the work on this specification is currently being done: WHATWG
  • The group where standardization of this work is intended to be done (if current group is a community group or other incubation venue): N/A
  • Major unresolved issues with or opposition to this specification: None known
  • This work is being funded by: Google

We'd prefer the TAG provide feedback as (please delete all but the desired option):

💬 leave review feedback as a comment in this issue and @-notify [@domenic, @shaseley ]

@cynthia cynthia self-assigned this Feb 1, 2022
@cynthia cynthia added Review type: later review Topic: DOM About or relating to the Document Object Model Topic: scripting ECMA, Web Assembly bindings, etc. Venue: WHATWG labels Feb 1, 2022
@torgo torgo added this to the 2022-02-07 milestone Feb 1, 2022
@plinss plinss self-assigned this Feb 2, 2022
@LeaVerou
Copy link
Member

LeaVerou commented Feb 8, 2022

We looked at this today, and had a question about the design of this API. Why is this a new factory method (especially on a non constructible class, AbortSignal) instead of a parameter in the AbortController constructor, that would also allow combining with regular signal functionality?

Also, we wondered if there are any plans for extending or cancelling the timeout.

@LeaVerou LeaVerou added the Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review label Feb 8, 2022
@domenic
Copy link
Member

domenic commented Feb 8, 2022

We looked at this today, and had a question about the design of this API. Why is this a new factory method (especially on a non constructible class, AbortSignal) instead of a parameter in the AbortController constructor

The idea is that there is no web-developer-visible AbortController for this AbortSignal; the browser holds onto the ability to control the signal, instead of the web developer. (In particular, the browser uses that ability to signal after the timeout.)

instead of a parameter in the AbortController constructor, that would also allow combining with regular signal functionality?

I'm not sure what you mean by this? In general combining works at the level of signals, not controllers.

Also, we wondered if there are any plans for extending or cancelling the timeout.

This was proposed in whatwg/dom#1039 but the use cases were rather esoteric. My current opinion is that it is not necessary to give such full control; if a developer wants that they can just use setTimeout + AbortController themselves. This is a simple factory method to make the 95% case easier, e.g. by not having the developer need to care about controllers, just getting a signal in a single line of code.

@torgo torgo modified the milestones: 2022-02-07, 2022-04-04-week Mar 30, 2022
@shaseley
Copy link
Author

Hi TAG folks, I wanted to provide a couple of updates on this:

  1. the DOM spec PR has been merged and Firefox and Safari have implemented this feature
  2. We've made progress on an API for combining signals, and as Domenic mentions, we envision scoping combining signals strictly to signals (not controllers + signals). I'll be filing a TAG review for that feature shortly.

Given (1) and the progress on (2) — which works nicely with this API — Chrome is planning to move forward with shipping this API, but look forward to considering any additional feedback as potential additions.

@plinss
Copy link
Member

plinss commented Jul 11, 2022

I accept that having the factory method on AbortSignal makes the simple case where an author only wants to timeout a fetch easy, e.g.:

 fetch(url, { signal: AbortSignal.timeout(8000) });

However, the scenario where the author needs their own AbortController and wants a timeout is more complex, e.g.:

const controller = AbortController();
fetch(url, { signal: AbortSignal.any(controller.signal, AbortSignal.timeout(8000)) });

vs having a timeout parameter on the AbortController constructor:

const controller = AbortController({ timeout: 8000 });
fetch(url, { signal: controller.signal });

Why not have both?

@shaseley
Copy link
Author

A small helper that uses AbortSignal primitives could also help clean this up:

const withTimeout = (signal, timeout) =>
  AbortSignal.any([signal, AbortSignal.timeout(timeout)]);

...

const controller = new AbortController();
fetch(url, {signal: withTimeout(controller.signal, 8000))};

// vs.

const controller = new AbortController({ timeout: 8000 });
fetch(url, { signal: controller.signal });

The timeout option seems reasonable to me, if DOM editors are supportive. I agree that the AbortSignal factory methods (.timeout() and proposed .any()), while sufficient (% perhaps timeout control), are not optimal for the case where a new controller is also needed. Avoiding the helpers here might be worth it.

Prior art on other platforms: .NET's CancellationTokenSource has a constructor timeout parameter and Go's context has a WithTimeout function, both of which provide similar functionality. Also, if necessary, AbortController could eventually be extended with methods to manipulate the timeout, similar to CancellationTokenSource.CancelAfter().

@nathanchase
Copy link

However, the scenario where the author needs their own AbortController and wants a timeout is more complex, e.g.:

const controller = AbortController();
fetch(url, { signal: AbortSignal.any(controller.signal, AbortSignal.timeout(8000)) });

This would also potentially be useful if you wanted an increase in timeout on subsequent request retries.

Example:
Request 1 - timeout 500 - retry
Request 2 - timeout 1000 - retry
Request 3 - timeout 2000 - retry
...

@plinss
Copy link
Member

plinss commented Aug 26, 2022

Any word if the constructor argument is being considered? (Is there an issue we can link to?)

@shaseley
Copy link
Author

shaseley commented Sep 9, 2022

@plinss I just filed an issue for this: whatwg/dom#1110.

@LeaVerou
Copy link
Member

Hi @shaseley, just circling back on this. Are there any updates? Thank you!

@LeaVerou
Copy link
Member

Hi @domenic,

We looked at this again today.
On the linked WHATWG issue you've made the comment:

I think this proposal is a bad idea and would be poor API design. The constructor of an object should vend its fundamental capabilities. It should not provide a syntactic shortcut for saving an extra line of setup, by connecting the constructed object to a totally different part of the web platform. (In this case, the event loop and timer subsystem.)

Furthermore, AbortControllers created in this way are harder to reason about. They have a hidden, implicit caller of abort(), which you cannot see. You have to know that whoever created it, hid such a call from you in the constructor. You can no longer scan for all abort() calls to find all ways the controller might be aborted.

I don't think we have evidence that the use case in question (roughly, creating an AbortController that is controlled by both a timeout and other callers) is very popular. And if we did, I don't think we have evidence that being explicit, using setTimeout(), is a huge burden. (Note that AbortSignal.any() is not actually needed, you can do const controller = new AbortController(); setTimeout(() => controller.abort(), 5000); and then pass controller onward.)

I hope we keep the AbortController constructor simple and focused on its core capabilities. Any syntactic sugar should continue to be done at the AbortSignal level, like we've done with AbortSignal.timeout() and like we're proposing to do with AbortSignal.any(). Static factory methods there both avoid messing up the API construct of the controller's constructor, and can pull their own weight by saving more than a single extra line (since they abstract away the entire AbortController object).

There are several design principles implicitly assumed in this message, and we'd like to discuss them separately, because we may or may not agree, and in any case it's good to get the benefit of a documented discussion that is separate from this particular issue.

For example, things like:

  • "The constructor of an object should not provide a syntactic shortcut for saving an extra line of setup"
  • "It should not [connect] the constructed object to a totally different part of the web platform. (In this case, the event loop and timer subsystem.)"
  • That having a hidden implicit call to abort() is a bad idea — why?

@cynthia cynthia added Resolution: timed out The TAG has requesed additional information but has not received it Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes and removed Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review labels Feb 9, 2023
@cynthia cynthia closed this as completed Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Resolution: timed out The TAG has requesed additional information but has not received it Review type: later review Review type: small delta Topic: DOM About or relating to the Document Object Model Topic: scripting ECMA, Web Assembly bindings, etc. Venue: WHATWG
Projects
None yet
Development

No branches or pull requests

9 participants