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

Rework handle ownership #2795

Merged
merged 11 commits into from
Feb 4, 2022
Merged

Rework handle ownership #2795

merged 11 commits into from
Feb 4, 2022

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Feb 1, 2022

Description

This is a major change that reshuffles the handle hierarchy quite a bit. It's very much a WIP, but would appreciate any early feedback.

  • Splits native handles into three distinct groups
    • Standalone types (StandaloneHandle) - e.g. User, App, ThreadSafeReference - those are types that exist on their own and have no ownership semantics with other types. Those are also typically not disposable.
    • Realm types (RealmHandle) - e.g. Object, RealmList, Results, etc. - those types are owned by a Realm instance and their lifetime is bound to that instance.
    • Realm (SharedRealmHandle) - this is a standalone type, but it takes care of unbinding its child handles at opportune times.
      • UnownedRealmHandle - this is a subtype of SharedRealmHandle that doesn't delete its native pointer because it wasn't the SDK that created it. Additionally, it keeps a weak list of all its children and closes them as soon as it gets closed. This is used in migrations, where we don't want to have objects outliving the Realm instance as this may keep the native instance alive and cause all sorts of issues.

Discussion items:

  • Currently, objects are bound to an instance, not native Realm. This means that disposing an individual instance will invalidate all objects that were created for it, even if the native Realm is still open:
    var realm1 = Realm.GetInstance();
    var realm2 = Realm.GetInstance();
    
    var obj = realm1.All<Foo>().First();
    realm1.Dispose();
    
    // obj is invalid here, even though the native Realm is still open
    I believe this is the intuitive and intended behavior, but its a breaking change compared to before (though the old behavior was accidental and not something we've ever officially supported).
  • Not a fan that we now need to call EnsureValid() on every handle method. It's easy to forget and if we do, attempting to call the method will throw a vague ObjectDisposedException. I tried a bunch of clever approaches, but wasn't happy with any of them. Open for smart ideas.
  • Right now, we don't ever prune the unowned handle list, which means that if there's a migration that instantiates millions of objects, we may run into trouble.

TODO

  • Changelog entry
  • Tests (if applicable)

@cla-bot cla-bot bot added the cla: yes label Feb 1, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1776006405

  • 227 of 239 (94.98%) changed or added relevant lines in 22 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.06%) to 82.913%

Changes Missing Coverage Covered Lines Changed/Added Lines %
Realm/Realm/Handles/ObjectHandle.cs 24 25 96.0%
Realm/Realm/Handles/StandaloneHandle.cs 8 11 72.73%
Realm/Realm/Handles/SetHandle.cs 19 23 82.61%
Realm/Realm/Handles/SharedRealmHandle.cs 35 39 89.74%
Files with Coverage Reduction New Missed Lines %
Realm/Realm/Exceptions/RealmException.cs 1 41.28%
Realm/Realm/Sync/FlexibleSync/SubscriptionSet.cs 4 94.44%
Totals Coverage Status
Change from base Build 1775866357: 0.06%
Covered Lines: 5519
Relevant Lines: 6542

💛 - Coveralls

@nirinchev nirinchev self-assigned this Feb 2, 2022
@nirinchev nirinchev marked this pull request as ready for review February 2, 2022 22:29
@@ -25,7 +25,7 @@

namespace Realms
{
internal class MongoCollectionHandle : RealmHandle
internal class MongoCollectionHandle : StandaloneHandle
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know about this part of the code. Is it a small subset duplicating what the c# driver does?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's nowhere near as comprehensive as the driver - the server exposes a REST-like API to query MongoDb and we're wrapping that here. But we're not querying MongoDb directly.

@@ -98,19 +98,26 @@ public bool IsValid
{
get
{
if (IsClosed)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't check for the root here?

@nirinchev nirinchev changed the title Reshuffle handles a bit Rework handle ownership Feb 4, 2022
@nirinchev nirinchev merged commit 416c3ab into main Feb 4, 2022
@nirinchev nirinchev deleted the ni/handle-fun branch February 4, 2022 13:44
nirinchev added a commit that referenced this pull request Feb 12, 2022
* main:
  Add notifications_cs.hpp to cmake lists
  Strip [UsedImplicitly] from the Remotion assembly (#2810)
  Allow setting primary keys during migration (#2793)
  fix typo (#2804)
  Remove various master references + fix up some stale docs (#2791)
  Rework handle ownership (#2795)
  Bump SharpZipLib from 1.3.2 to 1.3.3 in /Tools/SetupUnityPackage (#2798)
  Add a migration test that removes objects from the Realm (#2792)
@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.

3 participants