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.

Sign up for GitHub

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

fix one-tick-race condition in asyncMap #11249

Closed
wants to merge 3 commits into from
Closed

fix one-tick-race condition in asyncMap #11249

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e0695a
fix one-tick-race condition in `asyncMap`
phryneas Sep 25, 2023
984e474
changeset
phryneas Sep 25, 2023
591d6c1
size-limit
phryneas Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/hungry-vans-walk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

fixes a one-tick-race condition in `asyncMap`
11 changes: 6 additions & 5 deletions src/utilities/observables/asyncMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,21 @@ export function asyncMap<V, R>(
.then(both, both)
.then(
(result) => {
--activeCallbackCount;
next && next.call(observer, result);
if (completed) {
handler.complete!();
}
},
(error) => {
--activeCallbackCount;
throw error;
}
Comment on lines 34 to 40
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were not previously calling handler.complete() in the error handler, but now the promiseQueue.finally callback is potentially calling handler.complete() whenever we decrement activeCallbackCount. Is that a problem? Specifically, is it possible we might call error.call(observer, caught) and then also call complete.call(observer)?

)
.catch((caught) => {
error && error.call(observer, caught);
});
Copy link
Member Author

@phryneas phryneas Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The race condition essentially is the following:

  • handler.next (line 58) is called by the upstream Observable
  • code starts flowing in .then(both, both), both (the mapping function) throws an error (as it should in this specific scenario, because the result from the server contains errors and both is the function to handle that)
  • code reaches the --activeCallbackCount in line 43 and waits a tick before executing error.call(observer, caught)
  • meanwhile, handler.complete is called by the upstream Observable
  • activeCallbackCount is currently 0, complete.call(observer) executes
  • ⚠️ now it doesn't matter anymore if error.call(observer, caught) executes or not - the downstream observable is already complete (in our case without data, since we went into the error case, not the success case, so next was never called).

promiseQueue.finally(() => {
--activeCallbackCount;
if (completed) {
handler.complete!();
Comment on lines +45 to +48
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
promiseQueue.finally(() => {
--activeCallbackCount;
if (completed) {
handler.complete!();
promiseQueue.finally(() => {
--activeCallbackCount;
// Because of the .finally delay, it's possible handler.complete() gets
// called shortly before activeCallbackCount falls to 0, so each time we
// decrement activeCallbackCount we give handler.complete() another
// chance to call observer.complete().
if (completed) {
handler.complete!();

This might be a helpful comment, given the racy nature of this code.

}
});
Comment on lines +45 to +50
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix this, --activeCallbackCount has to be executed after next.call(observer, result)/error.call(observer, caught) above had a chance to execute.

I moved this out of the promise chain that is assigned to promiseQueue, as we don't need to queue up another tick in case promiseQueue is reused within a very short time span.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this logic, but I'm sure our future selves would appreciate a comment about why the .finally isn't just chained after the .catch.

};
} else {
return (arg) => delegate && delegate.call(observer, arg);
Expand Down
Loading