CoroutineLiveData randomly throws IllegalArgumentException [140249349]

Fixed

Bug

Status Update

No update yet.

Description

da...@gmail.com

created issue #1

Aug 29, 2019 08:06PM

Component used: lifecycle-livedata-ktx
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();
}
}
```

Comments

se...@google.com <se...@google.com> #2Aug 30, 2019 02:17PM

Also we need to add following for activities and fragments using VIewModel with to start:

## Android architecture components: ViewModel
-keepclassmembers class * extends android.arch.lifecycle.ViewModel {
<init>(...);
}

yb...@google.com <yb...@google.com> #3Aug 30, 2019 03:22PM

Reassigned to yb...@google.com.

Update:

Managed to get

https://github.com/googlesamples/android-architecture-components/tree/master/BasicSample working with the following set of rules:

## Android architecture components: Lifecycle
# generated GenericLifecycleObserver and it's empty constructor is considered to be unused by proguard
-keepclasseswithmembers class * implements android.arch.lifecycle.GenericLifecycleObserver {
<init>(...);
}
# keep Lifecycle State and Event enums with fields
-keepclassmembers class android.arch.lifecycle.Lifecycle$* { *; }
# keep methods annotated with @OnLifecycleEvent even if they seem to be unused
# (Mostly for LiveData.LifecycleBoundObserver.onStateChange(), but who knows)
-keepclassmembers class * {
@android.arch.lifecycle.OnLifecycleEvent *;
}
# ViewModel's empty constructor is considered to be unused by proguard
-keepclassmembers class * extends android.arch.lifecycle.ViewModel {
<init>(...);
}

Not quite sure if it's the final version (I think we can prettify it a bit), but everything seems to work fine.

yb...@google.com <yb...@google.com> #4Aug 30, 2019 06:22PM

Update:

https://github.com/googlesamples/android-architecture-components/tree/master/GithubBrowserSample also works fine with the same set of rules, added Okio, okHttp3, GSON and Dagger2 for Android rules

Please confirm it works fine for everyone.

I have a pull request for googlesamples/android-architecture-components ready, waiting for your feedback on this issue.

yb...@google.com <yb...@google.com> #5Sep 3, 2019 07:33PM

It's working fine for me, thanks a lot!

da...@gmail.com <da...@gmail.com> #6Sep 5, 2019 08:36PM

Update:

slightly modified the config I proivided:

## Android architecture components: Lifecycle
# LifecycleObserver's empty constructor is considered to be unused by proguard
-keepclassmembers class * implements android.arch.lifecycle.LifecycleObserver {
<init>(...);
}
# ViewModel's empty constructor is considered to be unused by proguard
-keepclassmembers class * extends android.arch.lifecycle.ViewModel {
<init>(...);
}
# keep Lifecycle State and Event enums values
-keepclassmembers class android.arch.lifecycle.Lifecycle$State { *; }
-keepclassmembers class android.arch.lifecycle.Lifecycle$Event { *; }
# keep methods annotated with @OnLifecycleEvent even if they seem to be unused
# (Mostly for LiveData.LifecycleBoundObserver.onStateChange(), but who knows)
-keepclassmembers class * {
@android.arch.lifecycle.OnLifecycleEvent *;
}

It turns out that generated GenericLifecycleObserver is unreachable after obfuscation because it is created using reflection by adding "_LifecycleAdapter" to LifecycleObserver class name (which is definitely changed). This leads to Lifecycle using fallback ReflectiveGenericLifecycleObserver (a little slower because of looking for your callback methods with reflection in order to call them). So we might just as well lose GenericLifecycleObserver generated by apt.

Another option is to keep both LifecycleObserver and generated GenericLifecycleObserver to improve performance. In that case replace
-keepclassmembers class * implements android.arch.lifecycle.LifecycleObserver {
<init>(...);
}

with

-keep class * implements android.arch.lifecycle.LifecycleObserver {
<init>(...);
}

yb...@google.com <yb...@google.com> #7Sep 6, 2019 12:49AM

This is still broken for me in alpha 2. I still need to specify ProGuard rules myself.

ap...@google.com <ap...@google.com> #8Sep 10, 2019 06:53AM

Looks like I missed part in our .gradle file.

yb...@google.com <yb...@google.com> Sep 18, 2019 04:12PM

Marked as fixed.