Fixed
Status Update
No update yet.
Comments
se...@google.com
I haven't looks on repro yet, but there is no such thing as AlwaysOnLifecycle in latest versions, LiveData has strong references on all observers (including that were added as observeForever). If nothings keeps a livedata, then it is gced.
yb...@google.com
ah I see: in observe with Lifecycle we add LiveData as observer to Lifecycle and it keeps it alive. However keeping observerforever + livedata, when nothing has reference on them, is a memory leak by definition: user can't clean this memory anyhow, because neither observer / livedata could be referenced.
yb...@google.com
It is true, though from usage perspective, it is not obvious at all.
e.g. you are in a class, like a singleton repository where you want to observe the database for some query.
and you write:
MyRepository(dao : MyDao) {
dao.someLiveData().observeForever(....do something...);
}
And it suddenly stops working :/ which is very non-obvious from the developer's perspective.
If we kept the lifecycle in memory as long as it has observers, wouldn't it solve the problem since they can break the chain by calling stopObserving(observer) ?
When they use observeForever, they are already responsible to remove the observer so I think it is fine?
e.g. you are in a class, like a singleton repository where you want to observe the database for some query.
and you write:
MyRepository(dao : MyDao) {
dao.someLiveData().observeForever(....do something...);
}
And it suddenly stops working :/ which is very non-obvious from the developer's perspective.
If we kept the lifecycle in memory as long as it has observers, wouldn't it solve the problem since they can break the chain by calling stopObserving(observer) ?
When they use observeForever, they are already responsible to remove the observer so I think it is fine?
yb...@google.com
well, the issue is: no reference on observer, you can't break the chain because you don't have a reference. If you have a reference, LiveData (unfortunately) wouldn't be gc-ed, because even AlwaysActiveObserver inrenally has strong reference on livedata.
da...@gmail.com
It turns out I had an app with the exact above use case: a singleton repository observing a database. The code was working properly with arch components 1.0.0 but unpredictably with 1.1.0. I think this is a good example of a leaky abstraction.
Since I want to observe the database forever with no intention to remove the observer at any point, I had no reason to keep a reference to the LiveData and it ended up being GCed along with the observers in it.
You could also argue that the problem is not with LiveData but with Room using weak references to observers but the developer is not supposed to know the Room implementation details.
Since I want to observe the database forever with no intention to remove the observer at any point, I had no reason to keep a reference to the LiveData and it ended up being GCed along with the observers in it.
You could also argue that the problem is not with LiveData but with Room using weak references to observers but the developer is not supposed to know the Room implementation details.
yb...@google.com
I think the real problem is that LiveData has no callback to know that no one wants it anymore; like onReset in Loader.
So there's 2 options for database observers:
1. It either has to un-register observer in onInactive and on next onActive re-query db. Problematic is if nothing has changed you have to filter values or dispatch unnecessarily, and you'll pay the price of query which could be expensive.
2. Register weak observer, or have wear ref to LiveData
Room uses option 2. I created my own LiveData and I used the same (https://bitbucket.org/snippets/Boza-s6/xe8749/sample-livedata-with-features-like-loaders#EmailContentLiveData.java-104 ), because it was obvious solution, but now I see it will not work in every possible case.
I think there should be statically referenced Lifecycle object to which LiveData subscribes when using observeForever() (and observer is anonymous class, maybe?).
So there's 2 options for database observers:
1. It either has to un-register observer in onInactive and on next onActive re-query db. Problematic is if nothing has changed you have to filter values or dispatch unnecessarily, and you'll pay the price of query which could be expensive.
2. Register weak observer, or have wear ref to LiveData
Room uses option 2. I created my own LiveData and I used the same (
I think there should be statically referenced Lifecycle object to which LiveData subscribes when using observeForever() (and observer is anonymous class, maybe?).
ap...@google.com
I ran into the same problem yesterday. It tooked me hours to understand what is happening and why my observeForever-observer suddenly stopped being invoked.
I think it is a really bad design that someone should know the implementation details of generated Room DB Code.
I want to use observerForever the same way like i use the simple observe and dont want to ran into problems because of internal implementation details.
I think it is a really bad design that someone should know the implementation details of generated Room DB Code.
I want to use observerForever the same way like i use the simple observe and dont want to ran into problems because of internal implementation details.
Description
Version used: 2.2.0-alpha01
Devices/Android versions reproduced on: Android 9
I'm having some random crashes due to CoroutineLiveData
```
Fatal Exception: java.lang.IllegalArgumentException: This source was already added with the different observer
at androidx.lifecycle.MediatorLiveData.addSource + 89(MediatorLiveData.java:89)
at androidx.lifecycle.CoroutineLiveDataKt.addDisposableSource + 102(CoroutineLiveDataKt.java:102)
at androidx.lifecycle.CoroutineLiveData.emitSource$lifecycle_livedata_ktx_release + 200(CoroutineLiveData.java:200)
at androidx.lifecycle.LiveDataScopeImpl$emitSource$2.invokeSuspend + 89(LiveDataScopeImpl.java:89)
at androidx.lifecycle.LiveDataScopeImpl$emitSource$2.invoke(LiveDataScopeImpl.java:11)
at kotlinx.coroutines.intrinsics.UndispatchedKt.startUndispatchedOrReturn + 91(UndispatchedKt.java:91)
at kotlinx.coroutines.BuildersKt__Builders_commonKt.withContext + 156(BuildersKt__Builders_commonKt.java:156)
at kotlinx.coroutines.BuildersKt.withContext + 1(BuildersKt.java:1)
at androidx.lifecycle.LiveDataScopeImpl.emitSource + 88(LiveDataScopeImpl.java:88)
at com.geekorum.ttrss.articles_list.FeedsViewModel$refreshed$1.invokeSuspend + 90(FeedsViewModel.java:90)
at com.geekorum.ttrss.articles_list.FeedsViewModel$refreshed$1.invoke(FeedsViewModel.java:8)
at androidx.lifecycle.BlockRunner$maybeRun$1.invokeSuspend + 147(BlockRunner.java:147)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith + 33(BaseContinuationImpl.java:33)
at kotlinx.coroutines.DispatchedTask.run + 241(DispatchedTask.java:241)
at android.os.Handler.handleCallback + 873(Handler.java:873)
at android.os.Handler.dispatchMessage + 99(Handler.java:99)
at android.os.Looper.loop + 193(Looper.java:193)
at android.app.ActivityThread.main + 6898(ActivityThread.java:6898)
at java.lang.reflect.Method.invoke(Method.java)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run + 537(RuntimeInit.java:537)
at com.android.internal.os.ZygoteInit.main + 858(ZygoteInit.java:858)
```
From what I understand of CoroutineLiveData source code, when calling `LiveDataScope.emitSource()` the previous source is removed before adding the new one on MediatorLiveData.
```
@MainThread
internal fun emitSource(source: LiveData<T>): DisposableHandle {
clearSource()
val newSource = addDisposableSource(source)
emittedSource = newSource
return newSource
}
```
However the removing is done by launching a new coroutine
```
internal fun <T> MediatorLiveData<T>.addDisposableSource(
source: LiveData<T>
): DisposableHandle {
val disposed = AtomicBoolean(false)
addSource(source) {
if (!disposed.get()) {
value = it
} else {
removeSource(source)
}
}
return object : DisposableHandle {
override fun dispose() {
if (disposed.compareAndSet(false, true)) {
CoroutineScope(Dispatchers.Main).launch {
removeSource(source)
}
}
}
}
}
```
So there is no guarantee that `MediatorLiveData.addSource()` will be called after the removing coroutine is executed. This leads to the IllegalArgumentException in `MediatorLiveData.addSource()`
```
@MainThread
public <S> void addSource(@NonNull LiveData<S> source, @NonNull Observer<? super S> onChanged) {
Source<S> e = new Source<>(source, onChanged);
Source<?> existing = mSources.putIfAbsent(source, e);
if (existing != null && existing.mObserver != onChanged) {
throw new IllegalArgumentException(
"This source was already added with the different observer");
}
if (existing != null) {
return;
}
if (hasActiveObservers()) {
e.plug();
}
}
```