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

Fix deadlock during manual client reset #7696

Merged
merged 7 commits into from
Jul 7, 2022

Conversation

clementetb
Copy link
Collaborator

Closes #7691

A deadlock will occur if we try to close the last Realm instance while inside a manual client reset block.

When we receive a sync error we acquire a lock on the Sync instance, while locked we are required to close all open Realm instances. This last instance would try to close the sync session, but it needs to acquire the lock that was previously acquired by the sync error logic.

It was not captured in our tests because the simulated sync error method did not simulate that the error came from a different thread than the one being executed.

This PR addresses this issue by not acquiring the lock on client reset errors.

@clementetb clementetb requested a review from nhachicha July 5, 2022 14:21
@clementetb clementetb self-assigned this Jul 5, 2022
@cla-bot cla-bot bot added the cla: yes label Jul 5, 2022
@clementetb clementetb changed the title Ct/fix deadlock manual client reset Fix deadlock during manual client reset Jul 5, 2022
@clementetb clementetb marked this pull request as ready for review July 5, 2022 14:32
Copy link
Collaborator

@nhachicha nhachicha left a comment

Choose a reason for hiding this comment

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

Nice catch 💯

@@ -307,7 +323,7 @@ synchronized void reset() {
* @param session Session to trigger Client Reset for.
*/
void simulateClientReset(SyncSession session) {
simulateClientReset(session, ErrorCode.DIVERGING_HISTORIES);
new Thread(() -> simulateClientReset(session, ErrorCode.DIVERGING_HISTORIES)).start();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a comment on why we run it on a separate thread (to simulate the error coming from the sync writer thread)

@clementetb clementetb merged commit 2661422 into releases Jul 7, 2022
@clementetb clementetb deleted the ct/fix-deadlock-manual-client-reset branch July 7, 2022 10:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants