airbnb / RxGroups

Easily group RxJava Observables together and tie them to your Android Activity lifecycle
Apache License 2.0
693 stars 44 forks source link

0.3.5: Observable not being cached and fragments / activities being leaked #30

Closed erseno closed 7 years ago

erseno commented 7 years ago

Hi there!

First, thank you for making this library, it looks like it will solve many issues with mobile applications when it comes to doing asynchronous work and sudden configuration changes.

However, I've run into an issue and I am not entirely sure what I am doing incorrectly as I believe I followed the sample closely

Introduction to app

Simple app that uses Retrofit to fetch a list of users. Once fetched, these users are displayed on a RecyclerView that is apart of a fragment

Summary of problems

My onStart method within the fragment looks like this

  @Override
    public void onStart() {
        super.onStart();
        if(mUsers == null && !mGroupLifecycleManager.hasObservable(TAG))
            loadUsers();
    }

So it checks if we do not have users already and the manager does not have the observable On a configuration change, the hasObservable call always returns false even though I am forwarding the appropriate life cycle calls

Once the task is done, it calls the following method in onNext

private void displayUsers(){
        showContent();
        UserAdapter userAdapter = new UserAdapter(mUsers);
        mRecyclerView.setAdapter(userAdapter);
        String randomString = getString(R.string.app_name);
    }

When I rotate the device whilst the task is ongoing, I will eventually I get the following error in my logcat

11-08 23:06:41.416 763-763/com.ersen.rxgroupissue I/UsersFragment: onError java.lang.IllegalStateException: Fragment UsersFragment{2fcd9c4} not attached to Activity
                                                                       at android.support.v4.app.Fragment.getResources(Fragment.java:648)
                                                                       at android.support.v4.app.Fragment.getString(Fragment.java:670)
                                                                       at com.ersen.rxgroupissue.fragments.UsersFragment.displayUsers(UsersFragment.java:107)
                                                                       at com.ersen.rxgroupissue.fragments.UsersFragment.access$200(UsersFragment.java:37)
                                                                       at com.ersen.rxgroupissue.fragments.UsersFragment$1.onNext(UsersFragment.java:131)
                                                                       at com.ersen.rxgroupissue.fragments.UsersFragment$1.onNext(UsersFragment.java:110)
                                                                       at rx.internal.util.ObserverSubscriber.onNext(ObserverSubscriber.java:34)
                                                                       at rx.observers.SafeSubscriber.onNext(SafeSubscriber.java:134)
                                                                       at rx.internal.operators.OperatorSubscribeOn$1$1.onNext(OperatorSubscribeOn.java:53)
                                                                       at rx.internal.operators.OperatorObserveOn$ObserveOnSubscriber.call(OperatorObserveOn.java:227)
                                                                       at rx.android.schedulers.LooperScheduler$ScheduledAction.run(LooperScheduler.java:107)
                                                                       at android.os.Handler.handleCallback(Handler.java:739)
                                                                       at android.os.Handler.dispatchMessage(Handler.java:95)
                                                                       at android.os.Looper.loop(Looper.java:148)
                                                                       at android.app.ActivityThread.main(ActivityThread.java:5417)
                                                                       at java.lang.reflect.Method.invoke(Native Method)
                                                                       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
                                                                       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)

I decided to look at a heap snapshot and I encountered that I had many MainAcitivity and UserFragment instances left in memory after configuration change. This is probably the cause of the above exception, somehow it called the method on a dead leftover instance of the fragment.

Here is a link to the project which replicates the issue I am having. I would be very grateful if you could look over this.

https://www.dropbox.com/s/ooipzvxqxeobipi/RxGroupIssue.rar?dl=0

Note that I forward the lifecycles to GroupLifecycleManager in the BaseFragment class

Any more information needed, let me know

Cheers!

felipecsl commented 7 years ago

Are you calling GroupLifecycleManager from all your lifecycle methods? Check out MainActivity in the sample app, you should be doing something similar

felipecsl commented 7 years ago

https://github.com/airbnb/RxGroups/blob/master/sample/src/main/java/com/airbnb/rxgroups/MainActivity.java

erseno commented 7 years ago

@felipecsl Hi, many thanks the reply. I will give a try this weekend and i'll report back my findings.

I am only forwarding the fragment ones but not the activity

felipecsl commented 7 years ago

that should be fine too

erseno commented 7 years ago

@felipecsl Hi, I double checked the lifecycle calls, I am calling them all for the fragment and activity. I am still experiencing the mentioned issues

BaseActivity

 @Override
    public void onCreate(@Nullable Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        Log.i(TAG,"onCreate");
        mGroupLifecycleManager = GroupLifecycleManager.onCreate(
                RxGroupApplication.getInstance().getObservableManager(),
                savedInstanceState, this);
    }

    @Override
    public void onPause() {
        super.onPause();
        Log.i(TAG,"onPause");
        mGroupLifecycleManager.onPause();
    }

    @Override
    public void onResume() {
        super.onResume();
        Log.i(TAG,"onResume");
        mGroupLifecycleManager.onResume();
    }

    @Override
    public void onSaveInstanceState(Bundle outState) {
        super.onSaveInstanceState(outState);
        Log.i(TAG,"onSaveInstanceState");
        mGroupLifecycleManager.onSaveInstanceState(outState);
    }

    @Override
    public void onDestroy() {
        super.onDestroy();
        Log.i(TAG,"onDestroy");
        mGroupLifecycleManager.onDestroy(this);
    }

BaseFragment

  @Override
    public void onCreate(@Nullable Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        Log.i(TAG,"onCreate");
        mGroupLifecycleManager = GroupLifecycleManager.onCreate(
                RxGroupApplication.getInstance().getObservableManager(),
                savedInstanceState, this);
    }

    @Override
    public void onPause() {
        super.onPause();
        Log.i(TAG,"onPause");
        mGroupLifecycleManager.onPause();
    }

    @Override
    public void onResume() {
        super.onResume();
        Log.i(TAG,"onResume");
        mGroupLifecycleManager.onResume();
    }

    @Override
    public void onSaveInstanceState(Bundle outState) {
        super.onSaveInstanceState(outState);
        Log.i(TAG,"onSaveInstanceState");
        mGroupLifecycleManager.onSaveInstanceState(outState);
    }

    @Override
    public void onDestroy() {
        super.onDestroy();
        Log.i(TAG,"onDestroy");
        mGroupLifecycleManager.onDestroy(getActivity());
    }

The link to the app has been updated too

felipecsl commented 7 years ago

OK @erseno I spent some time debugging your project to see what's going on, here's what I found out so far: You're doing:

userObservable
    .compose(mGroupLifecycleManager.<ArrayList<User>>transform(TAG))
    .delay(5, TimeUnit.SECONDS)

If you just invert the order of compose and delay, so that it looks like it's shown below, then it works:

userObservable
    .delay(5, TimeUnit.SECONDS)
    .compose(mGroupLifecycleManager.<ArrayList<User>>transform(TAG))

As a side effect, you'll have to manually move your code in onNext into the main thread since it will be called from a background thread.

This is just a workaround to get you unblocked, in the meantime I'll look into why this is happening. What I've seen so far is that ObservableGroup#add is causing the onTerminate callback to be invoked immediately after managedObservable#unlock is called. I'll keep digging on to why this is happening and will update this issue when I have a fix. Thanks for your great bug report and providing the test project.

felipecsl commented 7 years ago

OK so this is not a bug and working as intended. Remember that RxJava operators can't see the future, so they can't see anything that's applied after them. In this case specifically, the delay call. What's happening is that the Retrofit response is returning very fast (under 200ms average on my machine) and you're applying the RxGroups transformer on it. By the time that the response is received from Retrofit, RxGroups immediately removes that Observable from the list of "in flight" requests since it's already seen a terminal event (onCompleted). So, when you rotate, delay() is grabbing that event and holding onto it for 5 seconds, thus finally delivering it after the Fragment has been already destroyed, causing the Exception. When you move the delay call above compose, now RxGroups is aware of the entire stream and is able to correctly manage rotation since it will prevent delay() from delivering the event after onDestroy(). So TL;DR: not a bug 😄

erseno commented 7 years ago

Hi thanks for the comments and apologies for my late reply.

I've removed the delay call and made the emulator simulate a slow network connection (GPRS) so that the call takes a while. I am still encountering the same mentioned issues.

I've updated my project again.

Below the difference.

  Observable<ArrayList<User>> userObservable = RetrofitManager.INSTANCE.getApiService().getUsers();
        Observable<ArrayList<User>> userObservableTwo = RetrofitManager.INSTANCE.getApiService().getUsers();
        Observable<ArrayList<User>> userObservableThree = RetrofitManager.INSTANCE.getApiService().getUsers();
        Observable<ArrayList<User>> userObservableFour = RetrofitManager.INSTANCE.getApiService().getUsers();
        Observable<ArrayList<User>> userObservableFive = RetrofitManager.INSTANCE.getApiService().getUsers();

//        Observable.zip(userObservable, userObservableTwo, userObservableThree,userObservableFour,userObservableFive, (users, users2, users3,users4,users5) -> {
//            ArrayList<User> usersCombined = new ArrayList<>();
//            usersCombined.addAll(users);
//            usersCombined.addAll(users2);
//            usersCombined.addAll(users3);
//            usersCombined.addAll(users4);
//            usersCombined.addAll(users5);
//            return usersCombined;
//        })
//                .compose(mObservableGroup.<ArrayList<User>>transform(TAG))
//                .observeOn(AndroidSchedulers.mainThread())
//                .subscribeOn(Schedulers.newThread())
//                .subscribe(mObserver);

        userObservable
                .compose(mObservableGroup.<ArrayList<User>>transform(TAG))
                .observeOn(AndroidSchedulers.mainThread())
                .subscribeOn(Schedulers.newThread())
                .subscribe(mObserver);

The commented stuff was just to simulate a longer network request before I realised that it is possible for the emulator to simulate a slower internet.

Cheers!