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

Timeout AbortSignal (or AbortController) that gets input from the operation #1082

Open
annevk opened this issue May 23, 2022 · 6 comments
Open
Labels
topic: aborting AbortController and AbortSignal

Comments

@annevk
Copy link
Member

annevk commented May 23, 2022

Both whatwg/fetch#179 and whatwg/fetch#180 explain a desire for a timeout, but either the countdown gets reset based on networking activity (new chunk received) or it gets cancelled based on that (server was responsive within the allowed time).

Is that something fetch() should offer on its own or is there some decomposition of responsibility that could make sense here, whereby fetch() gets access to resetting or cancelling the timer.

@annevk annevk added the topic: aborting AbortController and AbortSignal label May 23, 2022
@domenic
Copy link
Member

domenic commented May 23, 2022

How I think of this:

Our base primitive is AbortController, which gives full programmatic control over when to abort, and with what reason.

We then add helpers for common cases where we can achieve a lot of simplicity. So AbortSignal.timeout() compresses us from

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

to just AbortSignal.timeout(ms).

The compression here would probably be something like:

const controller = AbortController();
let timerId = setTimeout(() => controller.abort(new DOMException("TimeoutError", "..."), ms);
onUpdate(() => {
  clearTimeout(timerId);
  timerId = setTimeout(() => controller.abort(new DOMException("TimeoutError", "..."), ms);
});
return controller.signal;

to

const { signal, reset } = createHelper(ms);
onUpdate(() => reset());
return signal;

I'm not sure this kind of compression is worth it, but maybe it is.

If it were added, I think it could be separate from any Fetch addition. I.e., any Fetch addition would probably build on top of such a helper, but not in a web-developer-visible way. Because the onUpdate(() => reset()) would be hidden inside the Fetch spec.

@jasnell
Copy link
Contributor

jasnell commented Jul 1, 2022

I think having a simple TimeoutAbortController type would be nice, but not critical.

class TimeoutAbortController extends AbortController { /* ... */ }

auto controller = new TimeoutAbortController(10 * 1000);
useSignalSomewhere(controller.signal);

// Call reset() to reset the timer.
controller.reset();

// TimeoutAbortController extends from AbortController, so still possible to call abort() directly.
controller.abort();

This would simplify some of the cases motivating the AbortSignal.any() discussion also, as the signal from a TimeoutAbortController could be triggered in two ways: either by the timeout or by explicitly calling abort().

@jasnell
Copy link
Contributor

jasnell commented Jul 30, 2022

Thinking about this more. I think what I'd like to see here are several additional APIs for both AbortController and AbortSignal.

  • AbortController.prototype.cancel() (or .close()) -- Indicates that the AbortSignal will never be triggered, signaling to it that any abort event handlers can be released.
  • AbortSignal.prototype.cancel() (or .close()) -- Like above.
  • AbortSignal.prototype.reset() -- Reset the AbortSignal to the intial state. If there is a timer, for instance, the timer is reset. If the signal is already triggered, resets the state so that it can be triggered again, clearing the stored reason.

I would go with either all three or only the last two.

Examples:

const signal = AbortSignal.timeout(10_000);
// ...
signal.reset(); // resets the timer to 10_000
//
signal.cancel(); // clears the timer, the signal will never trigger
const ac = new AbortController();
const signals = AbortSignal.any([ac.signal, AbortSignal.timeout(10_000)]);
ac.cancel();
// signals will only follow the timeout signal now

signals.reset();
// has no impact on the timeout signal if it hasn't already been triggered.
// signals will still trigger once the timeout completes.
// if signals has already been triggered, and both of the followed signals
// have either been canceled or triggered already, then signals will never
// trigger again after reset.

@keithamus
Copy link

I think it is very important for the signal to be unable to control upstream effects, such as remaining time. One of the motivations for timed AbortSignals is deadlines: "all work must complete in this given time, any response past then will be ignored". If a receiver of a signal is able to simply reset a deadline then it severely limits the efficacy of passing such a value.

@annevk
Copy link
Member Author

annevk commented Aug 25, 2022

Yeah, I think @keithamus is correct. For the scenarios in OP Fetch will have to own some kind of timer itself. There could also be a signal that is passed in (that ultimately wins) and the functionality might be implemented in terms of an internal signal Fetch controls, but the features seem kinda separate from the way signals normally work.

Adding more functionality to the controller seems reasonable, but I think we're already tracking that in other issues.

@bojavou
Copy link

bojavou commented Jul 5, 2023

I feel like there are cases where you want feedback to affect related operations.

Give up on a set of operations if nothing returns in time, otherwise they're still relevant so keep them all running.

const signal = AbortSignal.timeout(5000, { cancelable: true })

// Calculations detect a cancelable signal,
// cancel to keep the whole set running on first returned value
const stream = new CombineStream([
  generateNumbers(startFrom, Type.PrimeNumbers, { signal }),
  generateNumbers(startFrom, Type.SurrealNumbers, { signal }),
  generateNumbers(startFrom, Type.VampireNumbers, { signal })
])

Run 3 requests to the same server and keep them all alive as long as anything is returning data.

const signal = AbortSignal.timeout(5000, { resettable: true })

// fetch detects a resettable signal,
// and keeps it refreshed from all requests
const datasets = Promise.all([
  fetch('/WesternRegion.json', { signal })
    .then(response => response.json()),
  fetch('/EasternRegion.json', { signal })
    .then(response => response.json()),
  fetch('/SouthernRegion.json', { signal })
    .then(response => response.json())
])

It's really important not to make signals consumer-mutable by default. But where the operations are related, and you can guarantee the signal will never go to anything else, this is something I could use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: aborting AbortController and AbortSignal
Development

No branches or pull requests

5 participants