Remove Realm states dictionary #2251
Conversation
|
@nirinchev I've updated this branch by merging in |
nirinchev
force-pushed
the
ni/state-cleanup
branch
from
2463a3f
to
9210b97
Compare
| { | ||
| return r.TryGetTarget(out var other) && ReferenceEquals(realm, other); | ||
| }), "Trying to add a duplicate Realm to the RealmState."); | ||
| Debug.Assert(!GetLiveRealms().Any(other => ReferenceEquals(realm, other)), "Trying to add a duplicate Realm to the RealmState."); |
There was a problem hiding this comment.
Would it make sense to have this as a test instead of an assert?
There was a problem hiding this comment.
It's an assert because this is currently untestable behavior - there's no way currently to add the same realm twice as adding happens in the constructor. However, because there's nothing in the typesystem or the API design that enforces that, I wanted to add the extra check in case someone accidentally moves things around in the future. And it's a debug assert because the check isn't super cheap, so I didn't want us to do it in production.
| /// 4. Once the last instance is deleted, the CSharpBindingContext destructor is called, which frees the state GCHandle. | ||
| /// 5. The State is now eligible for collection, and its fields will be GC-ed. | ||
| /// </summary> | ||
| public void RemoveRealm(Realm realm) |
There was a problem hiding this comment.
According to the documentation this is called when Dispose is called:
- So the user does actually not call it themselves?
- Do we need it to be public then?
There was a problem hiding this comment.
The method is public, but the class it is defined in is internal:
realm-dotnet/Realm/Realm/Realm.cs
Line 1415 in 4b316aa
So it won't be exposed to end users.
|
There are no tests that need to be updated - the whole point of the states dictionary was to prevent deleting the Realm file while it's open. Since that is now achieved thanks to your work to move deletion in OS, the dictionary was actually unused - we were adding and removing elements, but not really using it for anything. |
nirinchev
force-pushed
the
ni/state-cleanup
branch
from
4b316aa
to
b17dff8
Compare
Description
While investigating #2224, I spent quite some time convincing myself that the Realm lifecycle is correct. While I couldn't find issues with it, I noticed that the
_statesdictionary approach was wrong. It relied on a ThreadLocal dictionary to hold the states for the current thread, but that is no longer safe since a Realm file may be opened/used across different threads as long as the synchronization context is the same. This was only used for the open Realm detection which should be moved to OS once realm/realm-core#4424 is merged.Fixes #2198 (doesn't do anything to fix it per se, but adds a thorough explanation why it's not a problem).
TODO