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 SyncSession crashing when accessed after receiving remote changes #1071

Merged
merged 5 commits into from
Oct 14, 2022

Conversation

cmelchior
Copy link
Contributor

Closes #1068

Note, when creating this patch I discovered another bug: #1070.

A test has been written that catch this but it is marked as @Ignore for now.

@cla-bot cla-bot bot added the cla: yes label Oct 13, 2022
@cmelchior cmelchior requested review from rorbech, edualonso, clementetb and nhachicha and removed request for edualonso October 13, 2022 19:12
Copy link
Contributor

@edualonso edualonso left a comment

Choose a reason for hiding this comment

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

Looks great, just one comment about the newly added test.

@@ -768,6 +836,7 @@ class SyncedRealmTests {
localRealm.writeCopyTo(flexSyncConfig)
}
}
flexApp.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// 4. With the original native dbPointer now being closed, accessing the syncSession for
// the first time should still work.
try {
realm1.syncSession.pause()
Copy link
Contributor

Choose a reason for hiding this comment

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

If an error happens here it will get swallowed by the finally and we won't know whether the session is still accessible. I would check the session state has actually been updated after calling pause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, finally doesn't swallow the exception, it should still propagate and fail the test, but you are right. Checking the state would improve the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I didn't mean an exception gets swallowed but rather that the state itself doesn't get updated for some reason. Poor wording 🙊

Christian Melchior added 2 commits October 14, 2022 11:19
@cmelchior cmelchior merged commit 1406032 into master Oct 14, 2022
@cmelchior cmelchior deleted the cm/bug/syncsession-crash branch October 14, 2022 12:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 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.

SyncSession.resume/pause crash after receiving remote update
2 participants