whatwg / dom

DOM Standard
https://dom.spec.whatwg.org/
Other
1.58k stars 295 forks source link

Consider adding a timeout parameter to the AbortController constructor #1110

Open shaseley opened 2 years ago

shaseley commented 2 years ago

Feedback from a TAG design review of AbortSignal.timeout() suggested adding timeout as a parameter to the AbortController constructor to improve ergonomics for the case where both a timeout and a controller is needed:

// controller.signal will be aborted after 5 seconds or sooner if `controller.abort()` is called.
let controller = new AbortController({timeout: 5000})`

I agree this would improve ergonomics for that case (even if we add AbortSignal.any()), and it still leaves open the possibility of adding timeout manipulation later (e.g. #1039). FWIW, as I mentioned in the TAG issue, there is prior art on other platforms for doing something like this, e.g. .NET's CancellationTokenSource has a constructor timeout parameter and Go's Context has a WithTimeout function, both of which have similar behavior to what's being proposed here.

Thoughts? Does this seem worth adding, or would it be better to consider this along with timeout manipulation and design them together?

tbondwilkinson commented 2 years ago

One unfortunate side-effect is that new option parameter are difficult to feature detect, vs. a static function property, which would suggest devs would need to fallback to UA sniffing to determine whether or not they have to handle timeout or have the option of a constructor parameter.

shaseley commented 2 years ago

Ah good point. I wonder here though if using EnforceRange solves that? If timeout was defined as unsigned w/ EnforceRange, which it is for AbortSignal.timeout(), I think new AbortController({timeout: -1}) would throw if the option is supported and wouldn't otherwise.

keithamus commented 2 years ago

A timeout of 0 could return an instantly aborted controller, so one could do feature detection:

const a = new AbortController({ timeout: 0 })
const supported = a.signal.aborted 

Failing that one can detect support with a getter:

let supported = false
const o = {}
Object.defineProperty(o, 'timeout', { get() { supported = true }})
const a = new AbortController(o)
annevk commented 2 years ago

Yeah, @keithamus is correct. Dictionary members can be detected quite well in most cases.

There's another thread on timeouts (see the issues with the same label) and one question I have is whether this should perhaps be a subclass of sorts as you probably also want to give the controller access to adjusting the timeout value, canceling it, and perhaps resetting it. Maybe it's okay to put all that on AbortController directly, but we'd have to think it through a bit.

domenic commented 2 years ago

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).

bathos commented 2 years ago

I’ve needed to compose timeout-related signals and other signals many times. In some cases this did mean just one timeout + its creator had controller access, but very often the relationships were more complex than that (or became more complex than that later) — e.g. branching where a signal from above (controller not passed down) could end up being one of multiple “abort factors” for its “descendants”.

We eventually ended up with a set of compositional utils for merging signals (and sometimes, but less frequently, controllers). These often had clear symmetry with static Promise methods. With a small library of those utils, managing AC/AS eventually became a breeze ... but it took time to land on the right mental model & understand what was really called for. This learning curve was reminiscent to me of the period years ago when lots of folks were fuzzy on how to use Promise effectively & tended to flip the resolvers/promise relationship around. If the Promise constructor had introduced a timeout argument then, I don’t think it would have helped the situation. In that analogy, controller.abort is more or less reject — it belongs to (or is) the “executor” and if I want to model an eventual time and an eventual [something else], I likely want two Promises or two AbortSignals.

(The absence of any, race, and so on though — that’s the stuff that really worked for us & kept the code clear over time — may be keeping this symmetry kinda hidden imo.)

adixon-adobe commented 1 year ago

I ran into this issue today. I definitely agree that the setTimeout logic isn't that burdensome, but the fact that AbortController uses AbortSignal internally, but that you can't actually use the AbortSignal.timeout() functionality was frustrating. The APIs very much look like something that should work.

I was actually looking to use AbortController to simplify some logic where I'm only using a timeout to abort a request that's related to some secondary page initialization logic for metrics. It's not critical that the request completes, so we want a short timeout on it.

To summarize: because AbortSignal has the timeout method, it feels broken that you can't use that with AbortController.

annevk commented 1 year ago

I think @bathos' comment is addressed with the addition of AbortSignal.any(). Perhaps we need more compositional methods, but those would be best tracked in their own issue.

@adixon-adobe AbortSignal doesn't have a timeout() instance method though. It has a static method that vends an AbortSignal(). That's quite different. Is it the presentation on MDN that makes this confusing?

I guess there's a question of whether there's room for a static that does something like the following

function controllerWithTimeout(timeout) {
  const controller = new AbortController();
  const timeoutSignal = AbortSignal.timeout(timeout);
  const signal = AbortSignal.any([controller.signal, timeoutSignal]);
  return { controller, signal };
}

and is perhaps a bit cleaner/magical (by flattening signal into controller), but I tend to agree with @domenic that there needs to be more evidence of need before we go there.

adixon-adobe commented 1 year ago

@annevk I still found myself wanting to pass in a parameter or custom AbortSignal that uses the static method. My point was that you can create an AbortSignal with a custom timeout value, and that's it's frustrating there's no way to do that in conjunction with AbortController because it manages that completely.

So I still think there's likely to be some frustration around this. And that doesn't necessarily mean it's worth prioritizing -- just want to make sure my feedback/experience is clear.