Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Question: Are AndroidLifecycleScopeProvider instances intended to be reusable? #114

Closed
bensandee opened this issue Oct 20, 2017 · 7 comments
Closed

Question: Are AndroidLifecycleScopeProvider instances intended to be reusable? #114

bensandee opened this issue Oct 20, 2017 · 7 comments
Labels
Android Bug
Milestone
0.4.0

Comments

@bensandee
Copy link
Contributor

bensandee commented Oct 20, 2017 •
edited
Loading

This is for AutoDispose 0.3.0.

A pattern I used for my custom scope providers for fragments/activities against AutoDispose 0.2.0 was to create a single scope provider (based on BehaviorSubject) and then share it between all observable chains for that entity. This worked fine, I think.

Now I'm migrating to the new AndroidLifecycleScopeProvider and I find that if I create a single instance for a fragment or activity, when, for example, a second onStart event occurs I get the exception below.

If I recreate the AndroidLifecycleScopeProvider instance for each subscription then it works fine, but it feels like it could be excess overhead. I do notice that this is the pattern you use in the samples, so perhaps it's intentional. There's no indication in AndroidLifecycleScopeProvider that it's stateful, so I'm just checking.

io.reactivex.exceptions.OnErrorNotImplementedException: Lifecycle has ended!
                         E      at io.reactivex.internal.functions.Functions$OnErrorMissingConsumer.accept(Functions.java:704)
                         E      at io.reactivex.internal.functions.Functions$OnErrorMissingConsumer.accept(Functions.java:701)
                         E      at io.reactivex.internal.observers.LambdaObserver.onError(LambdaObserver.java:77)
                         E      at com.uber.autodispose.AutoDisposingObserverImpl.onError(AutoDisposingObserverImpl.java:102)
                         E      at com.uber.autodispose.AutoDisposingObserverImpl$2.accept(AutoDisposingObserverImpl.java:57)
                         E      at com.uber.autodispose.AutoDisposingObserverImpl$2.accept(AutoDisposingObserverImpl.java:55)
                         E      at io.reactivex.internal.operators.maybe.MaybeCallbackObserver.onError(MaybeCallbackObserver.java:83)
                         E      at io.reactivex.internal.operators.maybe.MaybeDoOnEvent$DoOnEventMaybeObserver.onError(MaybeDoOnEvent.java:100)
                         E      at io.reactivex.internal.disposables.EmptyDisposable.error(EmptyDisposable.java:83)
                         E      at io.reactivex.internal.operators.maybe.MaybeDefer.subscribeActual(MaybeDefer.java:44)
                         E      at io.reactivex.Maybe.subscribe(Maybe.java:3727)
                         E      at io.reactivex.internal.operators.maybe.MaybeDoOnEvent.subscribeActual(MaybeDoOnEvent.java:39)
                         E      at io.reactivex.Maybe.subscribe(Maybe.java:3727)
                         E      at io.reactivex.Maybe.subscribeWith(Maybe.java:3793)
                         E      at io.reactivex.Maybe.subscribe(Maybe.java:3714)
                         E      at io.reactivex.Maybe.subscribe(Maybe.java:3680)
                         E      at com.uber.autodispose.AutoDisposingObserverImpl.onSubscribe(AutoDisposingObserverImpl.java:51)
                         E      at io.reactivex.internal.observers.BasicFuseableObserver.onSubscribe(BasicFuseableObserver.java:66)
                         E      at io.reactivex.internal.observers.BasicFuseableObserver.onSubscribe(BasicFuseableObserver.java:66)
                         E      at com.jakewharton.rxrelay2.PublishRelay.subscribeActual(PublishRelay.java:68)
                         E      at io.reactivex.Observable.subscribe(Observable.java:10910)
                         E      at io.reactivex.internal.operators.observable.ObservableFilter.subscribeActual(ObservableFilter.java:30)
                         E      at io.reactivex.Observable.subscribe(Observable.java:10910)
                         E      at io.reactivex.internal.operators.observable.ObservableMap.subscribeActual(ObservableMap.java:33)
                         E      at io.reactivex.Observable.subscribe(Observable.java:10910)
                         E      at com.uber.autodispose.ObservableScoper$AutoDisposeObservable.subscribeActual(ObservableScoper.java:113)
                         E      at io.reactivex.Observable.subscribe(Observable.java:10910)
                         E      at io.reactivex.Observable.subscribe(Observable.java:10896)
                         E      at io.reactivex.Observable.subscribe(Observable.java:10799)
                         E      at com.uber.autodispose.ObservableScoper$1.subscribe(ObservableScoper.java:71)
                         E      at com.orangebikelabs.BrowseFragment.onStart(BrowseFragment.java:222)
                         E      at android.support.v4.app.Fragment.performStart(Fragment.java:2380)
                         E      at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1458)
                         E      at android.support.v4.app.FragmentManagerImpl.moveFragmentToExpectedState(FragmentManager.java:1740)
                         E      at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1809)
                         E      at android.support.v4.app.BackStackRecord.executePopOps(BackStackRecord.java:857)
                         E      at android.support.v4.app.FragmentManagerImpl.executeOps(FragmentManager.java:2577)
                         E      at android.support.v4.app.FragmentManagerImpl.executeOpsTogether(FragmentManager.java:2367)
                         E      at android.support.v4.app.FragmentManagerImpl.removeRedundantOperationsAndExecute(FragmentManager.java:2322)
                         E      at android.support.v4.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:2229)
                         E      at android.support.v4.app.FragmentManagerImpl$1.run(FragmentManager.java:700)
                         E      at android.os.Handler.handleCallback(Handler.java:751)
                         E      at android.os.Handler.dispatchMessage(Handler.java:95)
                         E      at android.os.Looper.loop(Looper.java:154)
                         E      at android.app.ActivityThread.main(ActivityThread.java:6120)
                         E      at java.lang.reflect.Method.invoke(Native Method)
                         E      at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:865)
                         E      at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:755)
                         E  Caused by: com.uber.autodispose.LifecycleEndedException: Lifecycle has ended!
                         E      at com.uber.autodispose.android.lifecycle.AndroidLifecycleScopeProvider$1.apply(AndroidLifecycleScopeProvider.java:52)
                         E      at com.uber.autodispose.android.lifecycle.AndroidLifecycleScopeProvider$1.apply(AndroidLifecycleScopeProvider.java:38)
                         E      at com.uber.autodispose.ScopeUtil$3.call(ScopeUtil.java:91)
                         E      at com.uber.autodispose.ScopeUtil$3.call(ScopeUtil.java:74)
                         E      at io.reactivex.internal.operators.maybe.MaybeDefer.subscribeActual(MaybeDefer.java:41)
                         E      ... 39 more
@ZacSweers
Copy link
Collaborator

They should be reusable. What version of arch components are you using? Due to a behavior and API change, 0.3.0 is not compatible with rc1. I've just made some changes in #111 that fixes both, and this looks related to the behavior change (previously, after PAUSE, it would mark state as DESTROYED, but now it slinky's back up to STARTED)

@bensandee
Copy link
Contributor Author

I was using arch components beta1 because of that incompatibility with the rc1. The previous code that "worked" didn't use arch at all, it was just a custom scopeprovider with my own events. Your explanation makes sense.

I'll try out develop today and get back to you ASAP, hopefully to beat 0.4.0. Thanks!

@bensandee
Copy link
Contributor Author

I tried the current master with PR #111 but I still see the same issue. You can see for yourself in the sample app, modified slightly:

https://github.com/tbsandee/AutoDispose/commit/894f45202df17b5fc6edd789ce0cb7a369b4ce09

With this change, if you leave the app and come back (triggering onStop, onStart) the app will crash.

@ZacSweers
Copy link
Collaborator

interesting, I'll look into it. Thanks for the repro case!

@ZacSweers ZacSweers added this to the 0.4.0 milestone Oct 22, 2017
@ZacSweers
Copy link
Collaborator

So I understand the issue now, but not sure what to do about it. The issue is that the "last event" it sees when you resume in onStart is actually ON_STOP, which makes sense because that was the last seen event and the ON_START event is not emitted until after. We actually already have to do weird handling for this in the constructor of LifecycleEventsObservable to backfill events, but that is only done at initialization and not at resolution-time. We could make backfill callable from peekLifecycle, testing to see if this helps things.

ZacSweers added a commit that referenced this issue Oct 22, 2017
@ZacSweers
Make AndroidLifecycleScopeProvider reusable by backfilling from peek
b215346 
This moves backfilling out of a one-time thing in the constructor to something we do every time the lifecycle is peeked. It feels a bit weird though, but perhaps necessary because of the fact that arch lifecycle events are emitted _after_ the corresponding callback method.

Resolves #114
@ZacSweers ZacSweers mentioned this issue Oct 22, 2017
Merged
@ZacSweers
Copy link
Collaborator

#121 if you want to take a look. I'm not 100% sold on the solution, but also not sure if there's a better one

ZacSweers added a commit that referenced this issue Oct 23, 2017
@ZacSweers
Make AndroidLifecycleScopeProvider reusable by deferring backfill to …
31a55c5 
…peekLifecycle (#121)

* Make AndroidLifecycleScopeProvider reusable by backfilling from peek

This moves backfilling out of a one-time thing in the constructor to something we do every time the lifecycle is peeked. It feels a bit weird though, but perhaps necessary because of the fact that arch lifecycle events are emitted _after_ the corresponding callback method.

Resolves #114

* Fix imports

* Clean up doc
@bensandee
Copy link
Contributor Author

LGTM. Thanks for looking into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Android Bug
Projects
None yet
Milestone
0.4.0
Development

No branches or pull requests
2 participants
@bensandee @ZacSweers