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

Add a mutation queue to fix race conditions in toggles #1933

Merged
merged 19 commits into from
Nov 16, 2023
Merged

Add a mutation queue to fix race conditions in toggles #1933

merged 19 commits into from
Nov 16, 2023

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 16, 2023

This introduces a helper that lets us run "toggle" mutations (e.g. Follow/Unfollow) without races.

Currently, our approach is to treat each mutation as independent. However, their optimistic updates can clobber each other — e.g. if you press Follow immediately after Unfollow, you can't predict in which order they'll get to the server, or in which order they will come back. Our existing code did not deal with that.

Additionally, our API treats Unfollow/Unblock/Unmute as deletions of a record and requires a record URI. However, if you press Follow immediately followed by Unfollow (haha), it's "too early" to send the Unfollow request because we don't have any record URI for it yet. Our existing code tried to deal with that by guarding event handlers so that you can't Unfollow until Follow comes through — but that wasn't being applied consistently. And even if it was, you want to be able to "undo" an accidental Follow immediately.

In this PR, we take the following approach. The user is expected to be able to press the toggle as much as they like. The changes are applied immediately through our existing optimistic mutation mechanism. We still fire the underlying mutations via RQ. However, we don't "commit" or "rollback" the response immediately.

Instead, if you fire off multiple mutations immediately one after another, they run via a state machine that carries the returned server state through all the queued up actions one by one. This lets us feed the resource URI returned by the Follow action right into the pending Unfollow action. I chose to do it this way instead of reading the current shadow because I don't want to rely on the timing of React rendering (and updating the state) when orchestrating mutations. The state of the queue is reset when the queue is drained. So it only lives for as long as you keep smashing the button. This should reduce the surface area for bugs.

The queue automatically deduplicates pending actions. If you spam the button (e.g. Follow -> Unfollow -> Follow -> Unfollow -> Follow), as soon as it's done with the current request, it will apply at most one request if necessary. E.g. if we're waiting for Follow and the final requested state is also Follow, then there's nothing left to do. But if we're waiting for Follow and you requested Unfollow -> Follow -> Unfollow, the final Unfollow is still necessary — so we will apply just the one last Unfollow, and drop the intermediate actions.

API-wise, I opted to wrap our existing RQ mutations so that, if this abstraction is bad, it should be easy to rip out. At the callsites, I've removed the guards that protect against missing URIs (since the queue handles this now), but kept the overall structure the same. The individual Promises from queued calls resolve as the queue progresses, so we can still show toasts as individual requests come through, even if the entire queue is not drained yet. This provides feedback on slow network. I've ripped out spinners in the UI because now you can press Follow/Unfollow anytime and expect to get to the final state.

For testing, it might be helpful to turn on 3G emulation or to add manual Promise delays.

I only tested the web so far.

Review without whitespace


demo.mov

@gaearon gaearon requested a review from pfrazee November 16, 2023 19:59
updateProfileShadow(variables.did, {
followingUri: 'pending',
})
if (!variables.skipOptimistic) {
Copy link
Collaborator Author

@gaearon gaearon Nov 16, 2023

Choose a reason for hiding this comment

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

If we don't want to use useProfileFollowMutation and friends standalone then we can just rip these conditions out. However, I think it's plausible they might be useful somewhere else (or maybe the abstraction doesn't work out) so I kept them for now.

!profile.viewer?.following &&
profile.viewer?.following !== 'pending'
) {
if (profile.viewer?.following) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haven't tested this codepath. I did test the others.

did: profile.did,
blockUri: profile.viewer.blocking,
})
await queueUnblock()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strictly speaking, didn't have to convert this one since it stays modal.

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Yeah, looks great. +1

@pfrazee pfrazee merged commit 8475312 into main Nov 16, 2023
3 of 4 checks passed
@pfrazee pfrazee deleted the follow-q branch November 16, 2023 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants