trello-archive / RxLifecycle

Lifecycle handling APIs for Android apps using RxJava
Apache License 2.0
7.72k stars 637 forks source link

Possible bug in "bindUntilFragmentEvent(lifecycle(), event)" #19

Closed vekexasia closed 9 years ago

vekexasia commented 9 years ago

Hello there,

I started using the library after migrating from the old AppObservable. I experienced some weird crash (very sporadic) due to code being executed after the fragment view was destroyed. Then i tried to tie my tasks to PAUSE events and most of them went away but once in a while i experienced the same behavior.

After some digging and debugging my code i started to realize it is something related with this or the core library (not sure which one yet).

I discovered that sometimes you get the immediately subsequent event after the desired "STOP" FragmentEvent. Ex: If i want to receive events till onPause is called, sometimes i get an event immediately after onPause was sucessfully completed.

For this reason please consider this very basic fragment code

public class BauFragment extends RxFragment {
  PublishSubject<Integer> publishSubject = PublishSubject.create();

  public BauFragment() {
    lifecycle().subscribe(
        fragmentEvent -> {
          Timber.d("Fragment event is: %s - Thread: %s", fragmentEvent, Thread.currentThread());
        }
    );
  }

  @Override
  public void onPause() {
    Timber.d("onPause before super");
    super.onPause();
    Timber.d("onPause after super");
  }

  @Override
  public void onResume() {
    super.onResume();

    publishSubject
        .compose(RxLifecycle.bindUntilFragmentEvent(lifecycle(), FragmentEvent.PAUSE))
        .observeOn(AndroidSchedulers.mainThread())
        .subscribeOn(Schedulers.io())
        .subscribe(
            integer -> {

              Timber.d("Received %d, isResumed: %s, isRemoving: %s, thread: %s", integer, isResumed(), isRemoving(), Thread.currentThread());
            }
        );
    new Handler() {
      @Override
      public void handleMessage(Message msg) {
        publishSubject.onNext(msg.what);
        sendEmptyMessageDelayed(msg.what+1, 10);
      }
    }.sendEmptyMessage(1);
  }
}

It should look pretty straightforward. I'm trying to publish an integer through the publishSubject every 10ms ( bug appears also with 50ms or 100ms but is more sporadic ).

The generated output (depending how much time you let the fragment running before hitting the back button) should look similar to the following. (Note it does not always happen - as I said with 10ms you've more chances to get see it)

08-14 23:21:39.846 D/BauFragment﹕ Fragment event is: ATTACH - Thread: Thread[main,5,main]
08-14 23:21:39.847 D/BauFragment﹕ Fragment event is: CREATE - Thread: Thread[main,5,main]
08-14 23:21:39.848 D/BauFragment﹕ Fragment event is: START - Thread: Thread[main,5,main]
08-14 23:21:39.848 D/BauFragment﹕ Fragment event is: RESUME - Thread: Thread[main,5,main]
08-14 23:21:39.968 D/BauFragment﹕ Received 1, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.010 D/BauFragment﹕ Received 2, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.018 D/BauFragment﹕ Received 3, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.027 D/BauFragment﹕ Received 4, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.039 D/BauFragment﹕ Received 5, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.049 D/BauFragment﹕ Received 6, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.060 D/BauFragment﹕ Received 7, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.069 D/BauFragment﹕ Received 8, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.080 D/BauFragment﹕ Received 9, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
[--- Removed for clarity ---]
08-14 23:21:40.750 D/BauFragment﹕ Received 75, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.760 D/BauFragment﹕ Received 76, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.770 D/BauFragment﹕ Received 77, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.780 D/BauFragment﹕ Received 78, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.791 D/BauFragment﹕ Received 79, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.801 D/BauFragment﹕ Received 80, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:40.810 D/BauFragment﹕ Received 81, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:41.130 D/BauFragment﹕ Received 82, isResumed: true, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:41.194 D/BauFragment﹕ onPause before super
08-14 23:21:41.194 D/BauFragment﹕ Fragment event is: PAUSE - Thread: Thread[main,5,main]
08-14 23:21:41.194 D/BauFragment﹕ onPause after super
08-14 23:21:41.199 D/BauFragment﹕ Received 83, isResumed: false, isRemoving: false, thread: Thread[main,5,main]
08-14 23:21:41.565 D/BauFragment﹕ Fragment event is: STOP - Thread: Thread[main,5,main]
08-14 23:21:41.570 D/BauFragment﹕ Fragment event is: DESTROY_VIEW - Thread: Thread[main,5,main]
08-14 23:21:41.570 D/BauFragment﹕ Fragment event is: DESTROY - Thread: Thread[main,5,main]
08-14 23:21:41.571 D/BauFragment﹕ Fragment event is: DETACH - Thread: Thread[main,5,main]

Besides the headaches that this caused to me, it may be a "severe" bug if you bind to DESTROY_VIEW and, as I happened to see, sometimes an event goes through to the subscriber after onDestroyView gets called -> you'll likely to get a nullpointerexception (especially if you use Butterknife.unbind())

Before you ask these are my dependencies.

    // Rxjava stuff
    compile 'io.reactivex:rxjava:1.0.14'
    compile 'io.reactivex:rxandroid:1.0.1'
    compile 'com.trello:rxlifecycle:0.1.0'
    compile 'com.trello:rxlifecycle-components:0.1.0'
    compile 'com.jakewharton.rxbinding:rxbinding:0.1.0'
vekexasia commented 9 years ago

Not sure why by if I change these lines from

publishSubject
        .compose(RxLifecycle.bindUntilFragmentEvent(lifecycle(), FragmentEvent.PAUSE))
        .observeOn(AndroidSchedulers.mainThread())

to

publishSubject
        .observeOn(AndroidSchedulers.mainThread())
        .compose(RxLifecycle.bindUntilFragmentEvent(lifecycle(), FragmentEvent.PAUSE))

Everything works just fine....

dlew commented 9 years ago

This is a known problem - OperatorSubscribeUntil has a chance of leaking events based on where in the chain it's located. For more discussion, see https://github.com/ReactiveX/RxAndroid/pull/168

This should be solved by https://github.com/trello/RxLifecycle/pull/14. I'll work on getting a release out soon since I'd like to see how people react to the new behavior.

I'll leave this open until the next version is released so you can try it out and confirm it's been fixed.

vekexasia commented 9 years ago

ok thanks :)

dlew commented 9 years ago

I've pushed 0.2.0 to maven central - it should show up in a few hours. Let me know if that fixes the problem.

vekexasia commented 9 years ago

Hey @dlew I'll let you know tomorrow,

I read the changelog. Did I read wrong or I might expect receiving "onCompleted" after the target FragmentEvent has been triggered?

dlew commented 9 years ago

You will receive onCompleted. It's a change but it's necessary; there's no other way to solve this bug.

The alternative would be a complete rework of the system such that you subscribe in a special manner... which seems much less palatable. Most of the time getting onCompleted is not a big deal.