Better way to handle subscribing on non-UI thread

[exloong]

Android asks to call lifecycle.addObserver(observer); on the main thread, and here are the source code in LifecycleEventsObservable which I think have issues and can be better handled.

@Override protected void subscribeActual(Observer<? super Event> observer) {
    ArchLifecycleObserver archObserver =
        new ArchLifecycleObserver(lifecycle, observer, eventsObservable);
    observer.onSubscribe(archObserver);
    if (!isMainThread()) {
      observer.onError(
          new IllegalStateException("Lifecycles can only be bound to on the main thread!"));
      return;
    }
    lifecycle.addObserver(archObserver);
    if (archObserver.isDisposed()) {
      lifecycle.removeObserver(archObserver);
    }
  }

Issue:
Sending an error to Observable(and other equivalents such as Completable, Maybe and so on, I'll use Observable to represent them all) means it can be handled accordingly at runtime, but subscribe on a wrong thread cannot and shouldn't be handled at runtime. In this way, the errors are hide and redirected to a wrong channel. And it is not well mentioned in the documentation either, people can come across this issue accidentally without noticing (as someone might choose to swallow any errors from Obserable).

Possible solutions:

1. Mention all subscribe() should be called in main thread in documentation and add @MainThread to all subscribe() methods in ObservableSubscribeProxy class.

2. (Personally preferred) Switch to main thread inside of subscribeActual when calling lifecycle.addObserver(archObserver); and lifecycle.removeObserver(archObserver);

3. Throw IllegalStateException in subscribeActual() directly and crash the app, giving enough notice while debugging.

If you think it is really an issue and one of the solutions works, I am more than happy to implement them and create a pull request.

[ZacSweers]

What we do today is basically a form of #3, but we abide by the contract of routing to onError instead. We can document this more explicitly but option 1 isn't feasible (the core artifact is not an android artifact) and #2 would definitely be a footgun to users not expecting threading there

[exloong]

Thanks for the explanation and sorry for the duplicated issues as GitHub was running funny on the day I submitted the issue.

I agree with you, #2 might cause unexpected issues, I tried to wrap the LifecycleEventsObservable to allow auto thread switching in my case, but it was too hard to do as I couldn't find any entry points. Not sure if it is feasible or necessary to make the converter more customizable.

#3 can make a big difference when debugging in my case.
we found a failure that caused by the issue and handled by the default error handler in our app, as the subscribe() is in a method called by Observable.timer(), and can't really tell what was wrong until found a error message the log (luckily we printed them out while debugging). After that, I just realized the thread constraint and did a global check in our project, which has 59 occurrences, and more than 200 nested callers. And it took me around 4 hours for a medium size project, not too bad. But there would be no guarantee that newcomers, colleagues and I know and remember to call subscribe() on main thread. Crashing the app directly would be better for debugging, as it will not be mixed up with other expected errors and the cause can be easily found despite swallowing errors from observable. And ideally there would be centralized way to manage it.

[ZacSweers]

Unfortunately I can't give you a better option :/. This is the rxjava contract for its types, throwing instead would breach that contract (imagine if you couldn't get errors through onError).

[exloong]

Yes, I see, it seems like the only thing we can do is to put it into the document and be more careful when using it. I will probably put a @MainThread to every method which has autoDispose()