Fix useAbortableAsync race condition #207365
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flash1293
added
release_note:skip
This reverts commit e9d6585.
dgieselaar
approved these changes
Jan 21, 2025
|
Starting backport for target branches: 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jan 21, 2025
`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:  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:  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)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Jan 21, 2025
# Backport This will backport the following commits from `main` to `8.x`: - [Fix useAbortableAsync race condition (#207365)](#207365) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Joe Reuter","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-21T17:45:43Z","message":"Fix useAbortableAsync race condition (#207365)\n\n`useAbortableAsync` can easily get confused about the current state -\ne.g. when a previous invocation gets aborted and a new one is started at\nthe same time, the `loading` state gets set to false _after_ the next\ninvocation got started, so it's false for the time it's running:\n\n\n\n\nYou can see that while typing, the old slow request is aborted properly,\nbut the `loading` state gets lost and the abort error from the last\ninvocation is still set even though a new request is running already.\n\nThis is not the only possible issue that could happen here - e.g. if the\npromise chain throws too late, an unrelated error could be set in the\nerror handling logic, which is not related to the currently running\n`fn`.\n\nThis is hard to fix because as the hook does not control the `fn`, it\ndoes not know at which point it resolves, even after a new invocation\nwas started already. The abort signal asks the `fn` nicely to throw with\nan abort error, but it can't be controlled when that happens.\n\nThis PR introduces a notion of the current \"generation\" and only accepts\nstate updates from the most recent one.\n\nWith this, the new invocation correctly sets the loading state after the\nabort - what happens to the old promise chain after the abort can't\naffect the state anymore:\n\n\n\nI'm not sure whether this is the best way to resolve this issue, but I\ncouldn't come up with a better way. Happy to adjust, but I think we need\na solution that doesn't assume any special behavior of the passed in\n`fn`, otherwise this helper will always be super brittle.","sha":"8ff18e25758a05d4deff76ffa4b3407d98722a3c","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:version","v8.18.0"],"title":"Fix useAbortableAsync race condition","number":207365,"url":"https://github.com/elastic/kibana/pull/207365","mergeCommit":{"message":"Fix useAbortableAsync race condition (#207365)\n\n`useAbortableAsync` can easily get confused about the current state -\ne.g. when a previous invocation gets aborted and a new one is started at\nthe same time, the `loading` state gets set to false _after_ the next\ninvocation got started, so it's false for the time it's running:\n\n\n\n\nYou can see that while typing, the old slow request is aborted properly,\nbut the `loading` state gets lost and the abort error from the last\ninvocation is still set even though a new request is running already.\n\nThis is not the only possible issue that could happen here - e.g. if the\npromise chain throws too late, an unrelated error could be set in the\nerror handling logic, which is not related to the currently running\n`fn`.\n\nThis is hard to fix because as the hook does not control the `fn`, it\ndoes not know at which point it resolves, even after a new invocation\nwas started already. The abort signal asks the `fn` nicely to throw with\nan abort error, but it can't be controlled when that happens.\n\nThis PR introduces a notion of the current \"generation\" and only accepts\nstate updates from the most recent one.\n\nWith this, the new invocation correctly sets the loading state after the\nabort - what happens to the old promise chain after the abort can't\naffect the state anymore:\n\n\n\nI'm not sure whether this is the best way to resolve this issue, but I\ncouldn't come up with a better way. Happy to adjust, but I think we need\na solution that doesn't assume any special behavior of the passed in\n`fn`, otherwise this helper will always be super brittle.","sha":"8ff18e25758a05d4deff76ffa4b3407d98722a3c"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/207365","number":207365,"mergeCommit":{"message":"Fix useAbortableAsync race condition (#207365)\n\n`useAbortableAsync` can easily get confused about the current state -\ne.g. when a previous invocation gets aborted and a new one is started at\nthe same time, the `loading` state gets set to false _after_ the next\ninvocation got started, so it's false for the time it's running:\n\n\n\n\nYou can see that while typing, the old slow request is aborted properly,\nbut the `loading` state gets lost and the abort error from the last\ninvocation is still set even though a new request is running already.\n\nThis is not the only possible issue that could happen here - e.g. if the\npromise chain throws too late, an unrelated error could be set in the\nerror handling logic, which is not related to the currently running\n`fn`.\n\nThis is hard to fix because as the hook does not control the `fn`, it\ndoes not know at which point it resolves, even after a new invocation\nwas started already. The abort signal asks the `fn` nicely to throw with\nan abort error, but it can't be controlled when that happens.\n\nThis PR introduces a notion of the current \"generation\" and only accepts\nstate updates from the most recent one.\n\nWith this, the new invocation correctly sets the loading state after the\nabort - what happens to the old promise chain after the abort can't\naffect the state anymore:\n\n\n\nI'm not sure whether this is the best way to resolve this issue, but I\ncouldn't come up with a better way. Happy to adjust, but I think we need\na solution that doesn't assume any special behavior of the passed in\n`fn`, otherwise this helper will always be super brittle.","sha":"8ff18e25758a05d4deff76ffa4b3407d98722a3c"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Joe Reuter <[email protected]>
viduni94
pushed a commit
to viduni94/kibana
that referenced
this pull request
Jan 23, 2025
`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:  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:  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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
useAbortableAsynccan 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, theloadingstate gets set to false after the next invocation got started, so it's false for the time it's running:You can see that while typing, the old slow request is aborted properly, but the
loadingstate 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 thefnnicely 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:
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.