race condition in offline -> online state transition with dependent mutations

[jligeza]

Describe the bug

The react-query does not respect dependencies between offline-created mutations during their synchronization. They are fired all at once, which might cause race conditions in some cases.

The problem does not exist after state hydration, where queryClient.resumePausedMutations function makes the mutations fire one after another.

Your minimal, reproducible example

https://codesandbox.io/s/beautiful-voice-ykc42f?file=/src/App.tsx

Steps to reproduce

1. Open codesandbox link.
2. Disable network access using browser debugger to enter offline mode.
3. Create new post.
4. Submit it.
5. Restore network.

The draft post is created, but not submitted.

Expected behavior

A draft post should be created and submitted.
The submission mutation should wait for the creation mutation to finish.

How often does this bug happen?

None

Screenshots or Videos

No response

Platform

windows 10, Brave 1.47.186 Chromium: 109.0.5414.119

TanStack Query version

4.23.0

TypeScript version

4.9.3

Additional context

Created after submission of this stackoverflow question.

[TkDodo]

Not sure if there's a problem in your logic, but I looked at our code and resumePausedMutations is also what happens when you transition from offline to online without hydration. I've added some logs, and it shows that the api calls are actually happening one after the other:

• https://codesandbox.io/s/objective-sound-bgx7cr?file=/src/backend-mock.tsx

[jligeza]

This is the console output when I run the example according to the instructions:

createDraftPost
submitPost
resolved submitPost
null (this is an error from submission meaning post was not found, so it cannot be submitted)
resolved createDraftPost

The submit post should be printed after resolved createDraftPost. The mutations are fired all at once.

[TkDodo]

okay, why I am I constantly getting this output:

createDraftPost
resolved createDraftPost
submitPost
resolved submitPost

There is nothing I can see in code that would run the mutations in parallel 🤔

[jligeza]

After additional testing with a mobile phone (android with brave), I see the results are different, what makes me puzzled.

Looks like react-query works correct when using a phone, whenever I connect a debugger by USB or simply disable network in the device. The mutations execute in a sequence, avoiding the race condition.

It works incorrect in brave for PC (both windows and linux), where I use a debugger to disable network in the tab. For some reason I cannot trigger offline mode in any other way, because the web browser always returns navigator.onLine == true even while being offline.

[TkDodo]

Not sure what that means exactly - I've tested it successfully multiple times in Brave on MacOS. Gonna close this for now because I'm relatively sure that there is nothing wrong in react-query. I've looked at the code multiple times and it does run in serial :)

[jligeza]

After a little bit of extra testing, I managed to find a way to reproduce it on mobile. It can be reproduced both for mobile and PC.

There needs to be an extra step. Before you finally restore the network connection, you need to loose focus of the codesandbox tab. For example, detach the debugger and alt+tab to it. So, when you focus codesandbox again, but this time with network on, the mutations will fire all at once, creating a race condition.

On mobile, you can switch to another tab -> restore network -> go back to codesandbox tab. Putting brave in background and restoring it to foreground does not trigger the bug.

[TkDodo]

Thanks for clarifying. I can see what's going on:

• we go online, onlineManager tries to resumePausedMutations, starts resuming the first one.
• you focus the window, focusManager tries to resumePausedMutations as well, checks for all paused mutations, finds the second one (because the first one is no longer paused), and triggers it.

Now the real question is: Why would we ever want to resumePausedMutations in case of a focus event 🤔 . Maybe the fix is as simple as removing this line of code?

query/packages/query-core/src/queryClient.ts

Line 91 in 05a61b1

this.resumePausedMutations()

[TkDodo]

okay, I can see that we also pause if a mutation fails and the page is not focussed. If we want to keep that, we'd have to implement some kind of locking inside the mutationCache so that consecutive calls to resumePausedMutations wouldn't do anything.

[TkDodo]

@jligeza FYI, we're planning to revert this change because it introduced some other issues that are hard to fix. It is also not documented or intended that paused mutations execute in serial - they also wouldn't execute in serial if you just quickly created them while being online.

So per default, resuming paused mutations would trigger all mutations, in order, but we won't wait for them to finish before starting the next. Sorry for the confusion. I understand the use case of having dependent mutations, so if that's what you want, please take a look at the proposal for scoped mutations:

• RFC: Scoped Mutations #7126

Note that we won't revert this behaviour without also introducing the feature, so you will have a way to build up dependencies between mutations. As a plus, this will also be respected while you're online.

[jligeza]

Thank you for notice. This new solution will be an improvement, as there could be a situation where there are no dependent mutations paused, so once online we could execute them all in paralel, making an app faster.

[TkDodo]

Yep, that's exactly the idea of the change. The only difference is that it's opt-in, so if you are relying on the fact that all paused mutations run only after the previous one has been completed, you'd need to take care of this yourself now by putting them into the same scope.