getInstance refactor

[beeender]

The design of Realm/DynamicRealm/RealmCache is a long story, and also be noticed that there is a similar thing inside the Object Store which is doing the same for caching Realm instances called RealmCoordinator. We are not using it just because of RealmCache was added before that existed in Object Store, and there would be some breaking changes I would like to have to redesign the Realm instances mechanism:

• Semantics of Realm.close() Current Realm.close() is a confusing API, eg:

  Realm realm1 = Realm.getDefaultInstance();
  Realm realm2 = Realm.getDefaultInstance();
  realm1.close();
  print("Is realm1 closed?" + realm1.isClosed()) // false, confusing
  realm2.close();
  print("Is realm1 closed?" + realm1.isClosed()) // true, OK, the thread ref count reaches 0

it would be much clearer if:

  Realm realm1 = Realm.getDefaultInstance();
  Realm realm2 = Realm.getDefaultInstance();
  realm1.close();
  print("Is realm1 closed?" + realm1.isClosed()) // true
  print("Is realm2 closed?" + realm2.isClosed()) // false
  realm2.close();
  print("Is realm2 closed?" + realm1.isClosed()) // true

  // ...
  realm1.addChangeListener(listener1);
  realm2.addChangeListener(listener2);
  realm2.removeAllChangeListeners(); // The listener1 will be removed as well!!

We can just using the OS RealmCoordinator to achieve this quite easier, then also it is possible to have more debug information for users to check which specific Realm instance is leaked.

• DynamicRealm and Realm should Currently DynamicRealm and Realm can be created at the same time, and even on the same thread, they are using different SharedRealm instance. There was a reason that which i don't recall :grinning: And that creates complexity for the cache management. Also, it creates lot more corner cases we are not handled well right now: think about Realm instance and DynamicRealm instance both exist, then the DynamicRealm changes the schema and typed Realm won't be aware of that, the column indices will be wrong then. (I have seen a user on SO is doing that) This behavior should be change, allowing them both exist is not a valid use case at all, we should just disallow that.
• Garbage collect Realm instance by fixing the semantics of Realm.close(), we might be able to support GC better which is always a headache for users to remember to call close(). But this need to be investigated more since I don't really know if the GC is stressed enough to collect un-refed Realm instances on time
• Realm.deleteRealm() This API heavily relies on the RealmCache and it is not multi-processes safe. It is not ideal at all. But actually the implementation of it should exist in core to have a proper multi-processes support. See core issue https://github.com/realm/realm-core/issues/2165

we need to introduce some breaking changes in the API level (and I think they are good breaking changes) also a few dependencies from others.

[cmelchior]

We talked about using phantom references for Realm instances as well, which would make close() optional. The biggest drawback is that we might "leak" older Realm versions for a lot longer than desired. We would need to test this on real world apps in order to evaluate how such a change would affect file sizes.

At this stage in the Realm life cycle I would be very hesitant about changing how close() works. It might be a bit quirky, but changing the behaviour would be an extremely breaking change for a lot of apps, so the benefits would have to be massive I think.

I do generally agree with the concerns here, but it is changes we need to think really careful about.

[bmeike]

I would point out that the semantics of the Realm DB are very similar to those of Android's SQLite DBs.

You actually can close a SQLite DB, but the framework does everything it can, to hold one open, behind your back and return refs to the single cached instance for all requests.

The surprise with the built-in is that, if we both have refs to a DB, and I close mine, yours is closed. It is very difficult to get around this. Best practice suggests that you not close a DB without a very good reason.

The Realm approach of reference counting is just the other side of the coin. It seems somewhat friendlier to me. Not that we couldn't do better.... Bet current behavior is relatively easy to explain and I think people are used to it.

-blake

On Apr 27, 2017 8:07 PM, "Chen Mulong" [email protected] wrote:

The design of Realm/DynamicRealm/RealmCache is a long story, and also be noticed that there is a similar thing inside the Object Store which is doing the same for caching Realm instances called RealmCoordinator. We are not using it just because of RealmCache was added before that existed in Object Store, and there would be some breaking changes I would like to have to redesign the Realm instances mechanism:

• Semantics of Realm.close() Current Realm.close() is a confusing API, eg:

Realm realm1 = Realm.getDefaultInstance(); Realm realm2 = Realm.getDefaultInstance(); realm1.close(); print("Is realm1 closed?" + realm1.isClosed()) // false, confusing realm2.close(); print("Is realm1 closed?" + realm1.isClosed()) // true, OK, the thread ref count reaches 0

it would be much clearer if:

Realm realm1 = Realm.getDefaultInstance(); Realm realm2 = Realm.getDefaultInstance(); realm1.close(); print("Is realm1 closed?" + realm1.isClosed()) // true print("Is realm2 closed?" + realm2.isClosed()) // false realm2.close(); print("Is realm2 closed?" + realm1.isClosed()) // true

// ... realm1.addChangeListener(listener1); realm2.addChangeListener(listener2); realm2.removeAllChangeListeners(); // The listener1 will be removed as well!!

We can just using the OS RealmCoordinator to achieve this quite easier, then also it is possible to have more debug information for users to check which specific Realm instance is leaked.

DynamicRealm and Realm should Currently DynamicRealm and Realm can be created at the same time, and even on the same thread, they are using different SharedRealm instance. There was a reason that which i don't recall :) And that creates complexity for the cache management. Also, it creates lot more corner cases we are not handled well right now: think about Realm instance and DynamicRealm instance both exist, then the DynamicRealm changes the schema and typed Realm won't be aware of that, the column indices will be wrong then. (I have seen a user on SO is doing that) This behavior should be change, allowing them both exist is not a valid use case at all, we should just disallow that.

Garbage collect Realm instance by fixing the semantics of Realm.close(), we might be able to support GC better which is always a headache for users to remember to call close(). But this need to be investigated more since I don't really know if the GC is stressed enough to collect un-refed Realm instances on time

Realm.deleteRealm() This API heavily relies on the RealmCache and it is not multi-processes safe. It is not ideal at all. But actually the implementation of it should exist in core to have a proper multi-processes support. See core issue realm/realm-core#2165 https://github.com/realm/realm-core/issues/2165

So, yes, redesign is something I want to do as well, but then we need to introduce some breaking changes in the API level (and I think they are good breaking changes) also a few dependencies from others.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/realm/realm-java/issues/4573, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQ9WGiLAIxD5X9Yak2EKSU0XJWFOKmzks5r0VfjgaJpZM4NLALv .

[beeender]

I was thinking about some wicked ideas about auto close the Realm instance. The major problem is about Realm instances on the background thread, so if we know the thread has been terminated, it is actually safe to just close the Realm instance. There are only two key points of this:

1. When to check if the thread has been terminated.
2. What to do with the thread pool.

For 1) it is actually quite doable, we have a daemon thread to send notifications when Realm changes. When Realm is changed on any thread, it is actually a good timing to check if there are any thread Realm instances bond to are closed. So:

Thread thread = new Thread( -> {
    Realm realmBG = Realm.getInstance();
    // Do something...
};

// On the daemon thread:
{
    pipe.pollEvent(); 
    for (Realm realm : realms) {
        if (realm.thread.getState() == TERMINATED) {
            // realmBG will be closed here since the thread is terminated.
            realm.close();
        }
    }
}

For 2), I don't have good idea yet, since the thread won't be terminated if it was retrieved from a thread pool. Then only thing i can come up with is supply our own implementation of thread pool.