Skip to content

Commit

Permalink
Fix useAbortableAsync race condition (elastic#207365)
Browse files Browse the repository at this point in the history
`useAbortableAsync` can easily get confused about the current state -
e.g. when a previous invocation gets aborted and a new one is started at
the same time, the `loading` state gets set to false _after_ the next
invocation got started, so it's false for the time it's running:

![old](https://github.com/user-attachments/assets/6a784b1a-58b2-4951-8d25-9f109bce39c5)

You can see that while typing, the old slow request is aborted properly,
but the `loading` state gets lost and the abort error from the last
invocation is still set even though a new request is running already.

This is not the only possible issue that could happen here - e.g. if the
promise chain throws too late, an unrelated error could be set in the
error handling logic, which is not related to the currently running
`fn`.

This is hard to fix because as the hook does not control the `fn`, it
does not know at which point it resolves, even after a new invocation
was started already. The abort signal asks the `fn` nicely to throw with
an abort error, but it can't be controlled when that happens.

This PR introduces a notion of the current "generation" and only accepts
state updates from the most recent one.

With this, the new invocation correctly sets the loading state after the
abort - what happens to the old promise chain after the abort can't
affect the state anymore:

![new](https://github.com/user-attachments/assets/b39dd725-6bf1-4ef1-9eb6-d1463e1ec146)

I'm not sure whether this is the best way to resolve this issue, but I
couldn't come up with a better way. Happy to adjust, but I think we need
a solution that doesn't assume any special behavior of the passed in
`fn`, otherwise this helper will always be super brittle.

(cherry picked from commit 8ff18e2)
  • Loading branch information
flash1293 committed Jan 21, 2025
1 parent 8aa225d commit 325482b
Showing 1 changed file with 10 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -58,12 +58,17 @@ export function useAbortableAsync<T>(
const controller = new AbortController();
controllerRef.current = controller;

function isRequestStale() {
return controllerRef.current !== controller;
}

if (clearValueOnNext) {
setValue(undefined);
setError(undefined);
}

function handleError(err: Error) {
if (isRequestStale()) return;
setError(err);
if (unsetValueOnError) {
setValue(undefined);
@@ -78,11 +83,15 @@ export function useAbortableAsync<T>(
setLoading(true);
response
.then((nextValue) => {
if (isRequestStale()) return;
setError(undefined);
setValue(nextValue);
})
.catch(handleError)
.finally(() => setLoading(false));
.finally(() => {
if (isRequestStale()) return;
setLoading(false);
});
} else {
setError(undefined);
setValue(response);

0 comments on commit 325482b

Please sign in to comment.