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

Don't pass a shared_ptr to SyncManager to the sync client #4789

Merged
merged 3 commits into from
Jul 1, 2021

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Jun 29, 2021

What, How & Why?

Prevents a retain cycle on Apple devices where the sync manager would hold a strong reference to sync client which (through a lambda) would hold a strong reference to the sync manager. We're turning the sync manager reference to a weak one.

I'm not sure how to write tests for that and I'm sorry for the whitespace changes in the readme.

Fixes realm/realm-dotnet#2482

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)

@nirinchev nirinchev requested review from jbreams and fealebenpae June 29, 2021 18:30
@nirinchev nirinchev self-assigned this Jun 29, 2021
@nirinchev nirinchev requested a review from tgoyne June 29, 2021 18:30
Copy link
Member

@fealebenpae fealebenpae left a comment

Choose a reason for hiding this comment

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

I’m really not a fan of the whitespace changes in the changelog. Isn’t there a way not to reformat the whole file?

@nirinchev
Copy link
Member Author

I'm not a fan of trailing whitespace - happy to add a linter that adds markdown style rules and fails your builds 😛 but fine, I can revert them, although don't see a point in keeping it in.

@jbreams
Copy link
Contributor

jbreams commented Jun 29, 2021

Can you commit the whitespace changes under its own commit? I don't have a disapprove of formatting changes to try to make things cleaner, but including a bunch of unrelated changes in a commit that actually effects behavior is not good code hygiene. And please configure your editor not to do this automatically. For the change log, this is merely a little odd. However, it'd be really unfortunate if this were rewriting a bunch of unrelated code to satisfy your editor's whims.

@nirinchev
Copy link
Member Author

All of that is going to get squashed together anyway, so feels a bit pointless, but definitely not a hill I'm willing to die on. I reverted the whitespace changes as that was easiest and the Core team can decide between yourselves whether the trailing whitespace is worth keeping around or not.

@nirinchev
Copy link
Member Author

The failing tests are not related to this PR and will be addressed separately.

@nirinchev nirinchev merged commit 281af31 into v11 Jul 1, 2021
@nirinchev nirinchev deleted the ni/weak-sync-manager branch July 1, 2021 10:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 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.

4 participants