fix: Perform optimistic updates without awaiting #548
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
5d
approved these changes
Dec 6, 2023
harsh62
approved these changes
Jan 16, 2024
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.
Issue #, if available:
Within the SEEK application, there are occasions where performing a mutation's optimistic update would cause the main thread to lock up for long enough that our crash analytics considers it a hang (typically a couple of seconds), thereby crashing the application.
It's hard to determine an exact cause for this, and it's not at all helped by how rare the issue is. My educated guess is that something is attempting to come back to the main thread as part of performing the optimistic update, potentially within the SQLite layer that I don't have a lot of visibility into. Alternatively, it's possible that there is something like a file system delay that causes SQLite to wait for the file to be unlocked. Regardless, this means that performing a mutation on the main thread (fairly common in scenarios initiated by user interaction) mean that the main thread is locked, as
Promise.await()is called synchronously, and thus a semaphore is created.The resulting stacktrace (truncated to the relevant parts) looks something like this:
Description of changes:
In attempting to preserve the public ABI, I've opted to move the optimistic update into the mutation queue, and have removed a reliance upon the
await()method. This allows the mutation operation to be created (so it may be returned asCancellable), while ensuring it isn't actually added to the queue until after the optimistic update is performed.This allows the method to perform a mutation to return as quickly as possible, and avoids blocking the main thread.
Please note: I have not been able to successfully run integration tests (similar to past contributions), however we have validated that the issue is resolved by shipping the SEEK app using a fork with this change in place, and can confirm that the issue is eradicated.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.