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

Aligning realm references from writer results and public realm #481

Merged
merged 7 commits into from
Oct 6, 2021

Conversation

rorbech
Copy link
Contributor

@rorbech rorbech commented Oct 4, 2021

Closes #477

@rorbech rorbech requested a review from cmelchior October 4, 2021 14:00
Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 . This looks pretty easy to stumble into by accident, but not sure if there is a pattern we can employ that makes it easier to avoid mistakes. It can be in another PR anyway.

We should probably also update the changelog as this is an issue also present in our current release.

// cleared due some thresholds or outcome of GC not being predictable.
// https://github.com/realm/realm-kotlin/issues/486
@Ignore
fun clearingRealmObjectReleasesRealmReference() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test also failed before any of the reworks in this PR, so should IMO not prevent the actual fix of the PR to get merged. The test failing could just be a a symptom of our ability to reason about the outcome of GC on Native but could also be highlighting another issue. This was not checked before as the objects returned from write didn't actually reference the same version as we were tracking in RealmImpl. Added #486 to track investigation of this separately.

@rorbech rorbech requested a review from nhachicha October 6, 2021 06:33
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.

👏

@rorbech rorbech merged commit 8c9ec77 into master Oct 6, 2021
@rorbech rorbech deleted the cr/fix-erroneous-version-references branch October 6, 2021 09:39
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JVM/Android seems to release versions too soon
3 participants