Replace ugly runOnUiThreadIfFragmentAlive with Rx chain composition

[artem-zinnatullin]

If View methods return Action1 or probably Observable / Single will be able to compose them on the Presenter side and it'll work as part of initial chain.

Though, I also see some drawbacks, but in general, it's more functional and clear approach -> 👍

// Thanks @pakoito for the input!

[cxzhang2]

Hi @artem-zinnatullin , could you explain your choice to not .observeOn(AndroidSchedulers.mainThread()) in the first place? i.e.:

        final Subscription subscription = itemsModel
                .getItems()
                .subscribeOn(presenterConfiguration.ioScheduler())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(
                        items -> {
                            final ItemsView view = view();

                            if (view != null) {
                                view.showContentUi(items);
                            }

                            asyncJobsObserver.asyncJobFinished(asyncJob);
                        }, 
                      //error -> { ...etc etc

Seems like a lot of extra work to defer the .post call to runOnUiThreadIfFragmentAlive()?

Also, shouldn't the if(view != null) check be enough? Why do we still need to check isFragmentAlive() inside runOnUiThreadIfFragmentAlive()? I'm guessing it has something to do with the strange lifecycle callbacks of fragment, was just hoping for an explanation. Thank you very much!

[artem-zinnatullin]

Well, yeah, this is kind of thing I was not sure about when I initially wrote qualitymatters. At the moment I use MVVM mostly and we have a lot of Rx bindings UI code (abstracted from the Android platform) which heavily relies on info about main thread (but we inject it so it's changeable in the tests).

Feel free to submit a PR to change it to observeOn(mainThread) where main thread scheduler provided by DI.

On Wed, Apr 27, 2016 at 8:45 PM, Chris Zhang notifications@github.com wrote:

Hi @artem-zinnatullin https://github.com/artem-zinnatullin , could you explain your choice to not .observeOn(AndroidSchedulers.mainThread()) in the first place? i.e.:

    final Subscription subscription = itemsModel
            .getItems()
            .subscribeOn(presenterConfiguration.ioScheduler())
            .observeOn(AndroidSchedulers.mainThread())
            .subscribe(
                    items -> {
                        final ItemsView view = view();

                        if (view != null) {
                            view.showContentUi(items);
                        }

                        asyncJobsObserver.asyncJobFinished(asyncJob);
                    },
                  //error -> { ...etc etc

Seems like a lot of extra work to defer the .post call to the runOnUiThreadIfFragmentAlive() call in your views?

Also, shouldn't the if(view != null) check be enough? Why do we still need to check isFragmentAlive() inside runOnUiThreadIfFragmentAlive()? I'm guessing it has something to do with the strange lifecycle callbacks of fragment, was just hoping for an explanation. Thank you very much!

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/artem-zinnatullin/qualitymatters/issues/42#issuecomment-215168110

[artem-zinnatullin]

Soooooooo.

Was fixing two CalledFromWrongThreadException today in a real world project.

What was the reason you would ask?

Reason is simple, we have A LOT of rx streams, basically everything represented as rx streams, ui components like textview, buttons, etc are both streams and subscribers.

And the problem is that for performance and shortness of the code we don't add observeOn(mainThread()) everywhere we touch the ui and if you change the source observable which may be in different file/class/etc to emit data not on main thread you won't know if it's going to break the ui or not.

Two possible solutions:

1. Add observeOn(mainThread()) everywhere we touch the ui, basically all the streams we have in ViewModels (threat it as Presenters). Not only it's ugly but also not effective because it'll lead to overworkloading MainLooper, we also often combine ui streams together and it's not clear where is the best place to apply observeOn to make it both efficient and fail-safe.
2. Since we have abstraction of View and actual implementation we can make threading as part of View implementation which looks as much more fail-safe and easier to use model.

Just sharing my thoughts.

[pakoito]

All your Observables shouldn't end on UI but a data storage (behaviour subject, relay). Then, separately, subscribe those storages to the ui between resume and pause on the UI thread. Bam! no more threading issues. For intermediates use a functor operator to execute on specific schedulers (coming as a lib soonish).

That also solves state persistence for configuration changes, and allows for out-of-lifecycle subscriptions.

I've been evolving the architecture since last time we spoke, we need to catch up :D

[artem-zinnatullin]

The thing is that we combine a lot of streams in ViewModels, like clicks with data change and so on, we simple don't have that "storage only" flow, all the logic of the UI described as Rx steams.

[pakoito]

Stateless dumb UI driven by business logic streams. You still get to call the showSnack, setAdapterElements, changeScreen (view or frag, pick your poison), listen for back key presses, request other activities for result, and that stuff... yet they're completely write once, then fire & forget. And because they're Action1<T> or Observable<T> interfaces, mocking them is free. The only tricky one is recyclerview notifyDatasetWhatever to respect animations, specially with drag and drop callback, but it can be done. It's what Redux does on web, except multithreaded.

Definitely need to catch up.

[artem-zinnatullin]

We will catch up :)

Again, imagine all UI components are reactive, all clicks, text changes and so on. Then imagine how we work with them, we build combinations of those UI streams with data streams and then subscribe UI to the results, typical screen is about 10 combine(UI, data) streams on which we subscribe the UI so it displays what it should display.

And that's when threading may go wrong.

[pakoito]

All reactive UI: https://github.com/pakoito/SongkickInterview/blob/master/app/src/main/java/com/pacoworks/dereference/screens/songkicklist/SongkickListActivity.java

All reactive combinations: https://github.com/pakoito/SongkickInterview/blob/master/app/src/main/java/com/pacoworks/dereference/screens/songkicklist/SongkickListPresenter.java

And that's from 7 months ago, it's evolved since. If you want to avoid the madness and get the persistence you'll have to go through the stores.

[artem-zinnatullin]

Man, get yourself some lambdas!

This looks similar to what we have, observeOn is not a great solution for the reasons I described above. Very easy to break or catch performance problems.

[pakoito]

It's not in the middle of the chain, no, that's why you want a proxy in the state stores instead of applying directly to UI.

[artem-zinnatullin]

No dude, stay here as a man! Haha. I just think that somebody may find this useful, so let's keep it public.

What if you need streams that will be used by another streams? Where would you apply scheduling?

1. On initial stream — better performance, but nothing prevents you from removing that and breaking usages of this stream.
2. On each stream — sad performance and still possibility to forget observeOn before touching the UI.

[lenguyenthanh]

This conversation definitely should keep public. I'm facing similar problem. How do you guys think about this idea: http://panavtec.me/say-goodbye-to-all-main-thread-problems-in-mvp?

@artem-zinnatullin could you please share how you fix this problem with your real project? I'm really curious.

[artem-zinnatullin]

Fixed with observeOn but I and my co-worker want to move this to View layer.

On Fri, May 6, 2016 at 3:43 PM, Thanh Le notifications@github.com wrote:

This conversation definitely should keep public. I'm facing similar problem. How do you guys think about this idea: http://panavtec.me/say-goodbye-to-all-main-thread-problems-in-mvp?

@artem-zinnatullin https://github.com/artem-zinnatullin could you please share how you fix this problem with your real project? I'm really curious.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/artem-zinnatullin/qualitymatters/issues/42#issuecomment-217429280

[cxzhang2]

@pakoito , I'm looking at refreshData() in your SongkickListPresenter.java example, and I must be missing something because it seems like your observables "end on UI" and not "a data storage" as you say. I see that for every item emitted from the API you initiate a side effect to storeState(), but when you eventually .subcribe(getUi().swapAdapter()), you (seem) to be subscribing to the original API observable instead of this "state" that you're storing to. Please explain, thank you!

@artem-zinnatullin there is a danger to using observeOn(uiThread()) as detailed in this talk. I'm personally not sure of the best way to address this race condition, wrapping all my async observables in a BehaviorSubject doesn't sound fun. Maybe you can come up with a clever pattern!

[artem-zinnatullin]

@cxzhang2 actually I don't see that race conditions actually. Handler posts actions one after another and if you won't try to reorder things manually I don't see much possibilities for the race conditions.

[cxzhang2]

@artem-zinnatullin Pierre-Yves Ricau blogged about it here.

tldr: on your main looper:

1. post an orientation change request
2. post a runnable to setText on a TextView.
3. Boom.

[artem-zinnatullin]

I still don't see problem, if you unsubscribe from UI actions in onPause/onStop/onDestroy they will be removed from the Looper queue and no boom will happen.

More over I don't see how any solution that I saw in similar discussions are different from posting to main Looper, they're just moving posting code from one layer to another and call it "solution for async peoblem".

[cxzhang2]

You are right that posting to a BehaviorSubject alone will not change anything, I think the (implied) idea behind those solutions is that the BehaviorSubject will live inside a singleton. It's essentially the same solution that @pakoito is suggesting, i.e. having a data layer that's not bound to the activity lifecycle in between your source observable and your UI subscription.

Also, I was not aware that .unsubscribe() will remove any runnables it has previously inserted into the looper queue. If that is the case then the only benefit of the intermediate data layer (other than caching) will be to prevent loss of responses already in flight by the time an orientation change happens?

[artem-zinnatullin]

Basically you can do this with replay(1).autoConnect() and avoid BehaviorSubject because with behavior subject you usually can't subscribe it directly to the stream because if it encounter onCompleted() -> next time you subscribe to it it will just emit onCompleted, while replay(1).autoConnect() will emit last onNext even though stream was completed.

[cxzhang2]

Good call, thanks!

[artem-zinnatullin]

Ok, so I've deeply discussed this with team at Juno and we found really solid solution that solves 3 problems we have about ViewModel/Presenter -> View communication:

1. MainThread is part of View implementation.
2. Action posted to main thread is part of Subscription.
3. Backpressure on View layer should be detected and handled on ViewModel/Presenter layer.

I hope we'll publish this as separate library and implement it in QualityMatters + blog post from me.

[Plastix]

@artem-zinnatullin I look forward to reading about it!

[artem-zinnatullin]

So, here is the concept and implementation of the idea I was talking about: RxUi.

Will implement it in qualitymatters in some ~near future.

[lenguyenthanh]

awesome @artem-zinnatullin. Reading it.