It is possible for HandlerScheduler$ScheduledRunnable.run to still be called on a disposed object

[stevelilly]

I began investigating this as we have been seeing some strange crashes, where seemingly the only explanation was our subscriber was invoked after it was disposed (both the observe and the dispose in the main thread).

I have posted an example project here.
https://github.com/stevelilly/rxandroid-racecondition
Tested against rxandroid:2.0.2 + rxjava:2.1.13

This report is based on the belief that when running the following code in the main thread it should
not be possible to reach the exception:

Single.fromCallable(Object::new)
    .subscribeOn(Schedulers.io())
    .observeOn(AndroidSchedulers.mainThread())
    .subscribe(object -> {
        throw new IllegalStateException("should never be called");
    })
    .dispose();

After a few thousand calls I often encounter the exception. More interestingly, in the debugger
we can see that the HandlerScheduler$ScheduledRunnable has already been disposed, yet its run() method is still called.

AFAICT the code in the HandlerScheduler is using removeCallbacks and removeCallbacksAndMessages in the proper way, however it appears it is not safe to call delegate.run() without first checking disposed.

This seems to affect API 24+ much more often than API 23 when I test it, but this could just be the
specific timings of my emulator.

[JakeWharton]

It's not clear to me how disposed can be set to true from a call to dispose() on the main thread and the Runnable not be removed from the message queue of the main looper. We shouldn't need the proposed check since the main thread guarantees the ordering of these operations. Were this a background disposal the behavior would make sense since it becomes a race at that point (and we let the race happen inside the message queue rather than re-checking the boolean ourselves).

[artem-zinnatullin]

Looks like it happens due to call to DisposableHelper.replace() in SingleObserveOn.

Because of that call, call to HandlerScheduler.ScheduledRunnable.dispose() can actually end up on io thread (in this particular example), thus race with main thread that could already start executing the runnable.

A fix could be implemented either in RxAndroid (check disposed in run() as suggested) or in RxJava.
Or maybe actually in both, because RxJava's ScheduledRunnable.run also doesn't check for disposed and relies on proper race avoidance between disposing and executing which can be violated.

cc @akarnokd

I gotta run (pun) to the office and do actual work, hope that's enough info to start fixing the issue, will try to join efforts later.

@stevelilly thanks for reporting, providing code and sample project!

[JakeWharton]

I'm hesitant in calling that a fix as disposed could be set to true between the if and the call to run(). It certainly would reduce the probability of the errant behavior, but this behavior would still exist. I'm inclined to do nothing to avoid the extra volatile read which duplicates the race condition that's already happening inside of the main looper's message queue.

[akarnokd]

Yes, leave it to the looper.

[artem-zinnatullin]

Yeah I understand, but please consider following:

One could argue that both RxJava and RxAndroid should do everything reasonable to prevent something that is disposed from being executed.

In this particular case, as Jake originally noted race isn't even expected because it looks like we're disposing on the same thread we're scheduling to, but there is a race.

My rationale here is the following: we (RxJava, RxAndroid) schedule own ScheduledRunnable to underlying execution engine, we have a point before execution is delegated to user-passed Runnable. I think it's our responsibility to do everything reasonable to prevent its execution if possible.

If we'd directly schedule user-passed Runnable, well, then handler.removeCallback(userRunnable) or similar call to remove runnable from execution queue would be enough, but that's not the case.

People rely on Rx' ability of dealing with multithreading and race conditions, not adding this check might be treated as breach of trust

I can't call it a "fix" either, but I think it's something we can do to improve the situation :)

[JakeWharton]

If we flip the order of statements in dispose() you'll never observe disposed being set to true in our run(). The removeCallbacks method and the mechanism by which a Looper gets the head of the message queue is done with a lock. The disposed boolean is solely tracked for something to return from isDisposed(). If Handler had a hasCallback method we would delete the boolean.