Realm-java: A subtle change in timing in Realm 2.0.0-SNAPSHOT with handlers

[Zhuinden]

Goal

What do you want to achieve?

When a transaction is committed, a message was sent to the handler.
I'd like to send an event through event bus after Realm transaction is committed.

Expected Results

If I sent a message (event) of my own through the handler, then considering it was sent after the transaction, it ought to be received after the transaction's notification

Actual Results

_Sometimes_ the Realm updates later than the event through the event bus finishes, so findFirst() still returns null when event is received through the bus

Steps & Code to Reproduce

Describe your current debugging efforts.

Code Sample

realm.commitTransaction();
handler.post(new Runnable() {
                    @Override
                    public void run() {
                        bus.post(event);
                    }
                });

(obviously the solution is to use RealmChangeListener instead of relying on custom events, but it's still an important change because my old Realm code now has behavioral inconsistency)

Version of Realm and tooling

Realm version(s): 2.0.0-SNAPSHOT

Android Studio version: 2.1.2

Which Android version and device: Nougat 7.0.0 , Nexus 5X

[cmelchior]

Hi @Zhuinden, thanks for reporting this. This is most likely connected to us moving to another way of doing notifications in order to support changes from other processes.

I am not sure if it is a bug as such since we don't promise when the notification is delivered, but I do agree that any change in current functionality is unfortunate and the very least should be documented.

/cc @beeender

[Zhuinden]

@cmelchior yeah, I'd assume it's because REALM_CHANGED was removed in favor of the daemon thread doing its waitForChange() thing, so the daemon thread needs to detect the change before it sends its own notification, which can be both "sooner or later".

But the notion that Realm's notification would be delivered before any other event sent through the handler is not part of the contract.
It did make me wonder if this change in behavior influences https://github.com/realm/realm-java/issues/2990 (but that only seems to affect LOCAL_COMMIT which would mean it has no influence on background thread notifications).

[beeender]

Thanks a lot for the feedback.
Currently after transaction committed, the LOCAL_COMMIT will be sent immediately to the current thread like before. But when will the other threads get notified is not guaranteed ... It will be a problem for recycle views especially user wants to update only one subview after transaction in a worker thread :(

I think after this change, the refresh() actually becomes useful again:
1) User should be able to refresh the db to the latest db after transaction committed in another thread
2) The refresh() should trigger related listeners.
3) async queries are always a problem for manually refresh, in this case, if user decides to call refresh(), the async query should just be ran as a sync one I think?

I will start checking the async implementations in OS and try to figure out a unique behaviour for now and future.

@realm/java any ideas?

[cmelchior]

IMO, any attempt to "game" the Realm change notifications will contain race conditions, so it is not something we should encourage. Introducing refresh which then converts all your queries to synchronous ones doesn't sound like a good solution to me.

@Zhuinden Can you describe the use case where you need a custom handler event that cannot be covered by a standard RealmChangeListener?

[Zhuinden]

@cmelchior I primarily opened this issue just to document that this is a change in behavior.

It's our fault that we relied on an event bus callback to update the view after the transaction was committed on the background thread rather than just define the right RealmChangeListener (then again, the code was initially written for 0.81.1 so it's not _that_ surprising that we didn't always pick the right choices :tongue: )

So in reality, I don't _need_ the custom handler event. We'll just have to adapt and change to a proper RealmChangeListener to listen for changes.

---

A refresh() that auto-converts the async queries to sync queries sounds pretty bad though. Isn't that already avoided when REALM_CHANGED event was handled?

[Zhuinden]

You know, the real question is, does this alter the behavior of executeTransactionAsync(Realm.Transaction, Realm.Transaction.OnSuccess)?

[beeender]

@Zhuinden What we do for the pending async queries is when eg.: the UI thread gets a changed event and there is a pending async query for UI thread, we won't refresh the Realm immediately in that UI loop. Instead, the async query will be re-run in a worker thread and notify UI thread when it is done. So UI thread won't see the changes until the async query is finished.

But with refresh() this won't be possible anymore, since everything needs to be up-to-date immediately.

You know, the real question is, does this alter the behavior of executeTransactionAsync(Realm.Transaction, Realm.Transaction.OnSuccess)?

No, the behaviour of the onSuccess callback is the same. onSuccess is guaranteed to be called with the Realm instance refreshed to the version after that asyn transaction committed.

[Zhuinden]

@beeender well that answers it

@cmelchior Actually, I have the following code that relies on custom event:

        @Subscribe
        public void onGetSchedulesInteractorSuccessfulEvent(GetTVSchedulesInteractor.SuccessfulEvent e) {
            Date successDate = e.getDate();
            Calendar calendar = Calendar.getInstance();
            calendar.setTime(successDate);
            int successMonth = calendar.get(Calendar.MONTH);
            int successDay = calendar.get(Calendar.DAY_OF_MONTH);
            Date _currentDate = (currentDate == null) ? Now.getNow() : currentDate;
            calendar.setTime(_currentDate);
            int currentMonth = calendar.get(Calendar.MONTH);
            int currentDay = calendar.get(Calendar.DAY_OF_MONTH);
            if(successMonth == currentMonth && successDay == currentDay) {
                Log.d(TAG,
                      "Schedule interactor event received for date [" + successMonth + "-" + successDay + "], updating adapter.");
                setDatastructures();
                notifyDataSetChanged();
            }
        }

The schedules are downloaded for 6 days, each a different call, and currently this updates the UI only if the downloaded schedules belong to the currently selected day.

So in this code it's to avoid calling setDatastructures() for _every_ Realm table change.

[cmelchior]

Ok, so it is mainly a workaround for our limited RealmChangeListeners? and Fine-grained notifications would solve this issue for you?

[Zhuinden]

@cmelchior I... think so.

I don't really know how fine-grained the fine-grained notifications are yet, but I think if it means that the RealmChangeListener is not called unless the results inside it have also changed, then it could work

[cmelchior]

Fair point :), but yes, the idea is that the RealmChangeListener on any RealmResults is only triggered if the content of the RealmResults changed somehow. Right now our table-wide notifications will trigger all RealmChangeListeners for a given type.

[Zhuinden]

@cmelchior I can define a result set that monitors schedules for a given day, so that would work.

I'll just have to rework the code a bit I guess when 2.0 stabilizes

[Zhuinden]

@cmelchior @beeender but you know, with people using IntentServices for background threading, I sometimes feel like removing refresh() on non-looper threads was a mistake.

[Zhuinden]

@cmelchior if there's one more issue with it, I can also use the events as a confirmation of sorts, like OnSuccess in executeTransactionAsync (but I am on background thread so it's obviously just executeTransaction.

So if I wanted a popup opened by an event through an event bus which says "successfully added blah and blah (data from the item), I wouldn't be able to show this data directly via a Realm query, because there's no guarantee the item is actually there, but synchronizing state via a boolean flag saying "transaction is in progress" and handle the boolean state in the change listener just seems racy.

Sending copied data through the event bus might be a solution, but it'd be odd to rely on the user for "artificial delay" so that the item actually is there when he pressed the button.

So if there is a change from a background thread for a specific item and I want to do something only once when it happens, how would I do that now reliably? Introduce delay on the event sent through the handler? Add waitForChange before sending the event?

I don't want to Rx it just so I can take the first Realm change :tongue:

[Zhuinden]

@beeender But we do know this is because REALM_CHANGED was supposedly no longer necessary because the Realm daemon is notified on changes from any thread and any process, but as a result, the update on the UI thread depends on waitForChange to notify the daemon, and doesn't immediately notify the UI thread's handler for transactions in the same process.

But now I'm not sure how to notify the UI thread only once after the transaction is complete in the background. Executing empty async transaction and using its onSuccess would work, but that's a strange API.

[Zhuinden]

Welp, the same issue occurs in onPostExecute of AsyncTask

[Zhuinden]

@cmelchior so I realized I was actually thinking of this issue with https://github.com/realm/realm-java/issues/3476#issuecomment-252572034

But the change _can_ already be synced by the time onPostExecute() is called, just _not necessarily_ :slightly_frowning_face:

[Zhuinden]

So I was thinking about this and I still have no idea what to do about it. There is no way to guarantee that I can think of when you execute a transaction on a background thread such as doInBackground(), then in onPostExecute(), the Realm would be up-to-date 100% times.

[optisamit]

@Zhuinden But this works for realm < 2.0. Thus, there should be a way, IMHO

[Zhuinden]

@optisamit yes and no, it works for Realm < 2.0 because the way background thread transaction notifications were handled were intra-process, aka they used the Android Handler to post a message.

Now they need to notify the UI thread through the Object Store's own notification system, which inevitably adds delay instead of directly posting a message.

---

Honestly, this won't really be a problem though once fine-grained notifications are added, although I do admit that committing in an AsyncTask and then querying in onPostExecute() will give inconsistent results.

[cmelchior]

I'll close this issue. We never documented the timing as part of the official API and as such it should be considered an implementation detail.

RealmChangeListeners should be used to be notified about changes, and if they are not sufficient we should fix those instead.

[optisamit]

@cmelchior I'm sorry, but it IS documented:

The other objects in the other threads will be updated in near real time (meaning that if they’re currently in the process of doing something the update will happen in the next iteration of the runloop).

https://realm.io/docs/java/2.3.0/#threading

[Zhuinden]

@cmelchior And this also means that the stuff mentioned for AsyncTask in https://realm.io/docs/java/2.3.0/#strategies-when-dealing-with-android-framework-threads does not work reliably, because in onPostExecute() there is no guarantee that the Realm instance is actually up to date about 2 out of 3 times

[cmelchior]

@optisamit

The other objects in the other threads will be updated in near real time (meaning that if they’re currently in the process of doing something the update will happen in the next iteration of the runloop).

That is an imprecise description of what is happening and it wasn't actually true even before this issue. I'll update the docs.

The Looper queue consists of a lot of other events, in particular touch events, so it was also possible to run into this problem before.

[Zhuinden]

What about the AsyncTask example in the docs? onPostExecute() has no guarantee of the Realm instance being up to date.

[cmelchior]

@Zhuinden Yes, we will have to go through the async example and make sure it is still accurate or need updating.

[Zhuinden]

In which case this issue is still open for DOC

edit: added at https://github.com/realm/realm-java/issues/4098