Race condition between Realm change notifications and UI events #2990
Conversation
* Add param to @RunTestInLooperThread to make it possible to populate db before test starts in looper thread. * More test cases for this PR.
…ueue Conflicts: CHANGELOG.md realm/realm-library/src/androidTest/java/io/realm/NotificationsTest.java
Conflicts: CHANGELOG.md realm/realm-library/src/androidTest/java/io/realm/NotificationsTest.java realm/realm-library/src/main/java/io/realm/HandlerController.java
cmelchior
changed the title
Conflicts: CHANGELOG.md
| Message msg = Message.obtain(); | ||
| msg.what = HandlerController.LOCAL_COMMIT; | ||
| if (!handler.hasMessages(HandlerController.LOCAL_COMMIT)) { | ||
| messageHandled = handler.sendMessageAtFrontOfQueue(msg); |
There was a problem hiding this comment.
It would be unharmful to do a handler.removeMessage(HandlerController.REALM_CHANGED); here. But it is quite minor. I am fine without it.
There was a problem hiding this comment.
Since it goes against the recommendations, we could write a message to the log. It could ease debugging later.
There was a problem hiding this comment.
I would much prefer not flooding the log with this, but we could add a debug log message just like we do for a lot of the other async callbacks. That would leave them disabled by default, but could be enabled if we suspected it.
|
👍 looks good! and i cannot imagine another error case by now :P |
| // The presence of async RealmResults block any `REALM_CHANGE` notification causing historically the Realm | ||
| // to advance to the latest version. We make sure in this test that all Realm listeners will be notified | ||
| // regardless of the presence of an async RealmObject that will delay the `REALM_CHANGE` sometimes | ||
| // The presence of async RealmResults block any `REALM_CHANGE` notification . We make sure in this test that all |
|
I can only say it makes lot of sense in unit test, but I'd say we could appreciate @nhachicha's review on this. LGTM 👍 |
| "to prevent this."); | ||
| Looper looper = handler.getLooper(); | ||
| if (realmPath.equals(configuration.getPath()) // It's the right realm | ||
| && looper.getThread().isAlive()) { // The receiving thread is alive |
| // | ||
| @Test | ||
| @RunTestInLooperThread(/*step1*/PopulateOneAllTypes.class) | ||
| public void realmListener_localChangeShouldBeSendAtFrontOfTheQueueWithPausedAsync() { |
There was a problem hiding this comment.
How's this test different from the previous one?
I don't see where you pause the async query, same for the previous one what do you mean by LoadedAsync? is it calling load?
There was a problem hiding this comment.
Realm.asyncTaskExecutor.pause(); is pausing it.
The difference is that the first test loads the async query first, while the second one pauses the query so we hit two different codepaths for async RealmResults. The difference is step 2, but yes, I agree it is slightly hidden.
|
LGTM small comment 👍 |
Conflicts: CHANGELOG.md
Fixes realm/realm-android-adapters#11
Fixes realm/realm-android-adapters#48
From #2952
This PR fixes a bug surfaced by our Realm Android Adapter example. Before this PR we posted all REALM_CHANGED events to the looper queue as normal. This meant that potential touch input events could get in front which in turn could cause a relayout of a ListView which would then crash because it detected a difference between it's cached adapter size and the actual one.
This PR now forces all local commits to be placed at the front of the queue eliminating that risk.
Using postAtFrontOfQueue is highly discouraged by Google since you risk starving the UI thread and introduce lag. However commits are generally fairly infrequent and any loop (commits inside a change listener) will now be more easily detected. Before this, they would probably work, although making the UI sluggish.
Implementation is as follows:
RealmResultsImplementation comment:
Ideally on LOCAL_COMMIT we want to stop any non-loaded queries as they will fail when reaching the Looper and will need to retry anyway. For async queries that are already loaded, we don't need to reschedule as they will be synced on the Looper thread. However currently it is not possible to make that distinction, and I would rather keep this out of scope for this PR, as it would be a bigger change for little benefit.
For that reason I am instead just letting any un-loaded queries run to completion including retry.