Skip to content

Remove handler callbacks before setting disposed.#425

Merged
JakeWharton merged 1 commit into2.xfrom
race
Jul 1, 2018
Merged

Remove handler callbacks before setting disposed.#425
JakeWharton merged 1 commit into2.xfrom
race

Conversation

@JakeWharton
Copy link
Copy Markdown
Contributor

This change ensures that you can never observe disposed being set to true inside run(). removeCallbacks and the mechanism by which a Looper retrieves the head of the message queue are both governed by a lock. When dispose() is called on a non-main thread taking this lock is the race which determines cancelation and not the boolean.

Include a comment emphasizing that the boolean is tracked solely for the purposes of accurate isDisposed() reporting.

Refs #424.

This change ensures that you can never observe `disposed` being set to `true` inside `run()`. `removeCallbacks` and the mechanism by which a `Looper` retrieves the head of the message queue are both governed by a lock. When `dispose()` is called on a non-main thread taking this lock is the race which determines cancelation and **not** the boolean.

Include a comment emphasizing that the boolean is tracked solely for the purposes of accurate `isDisposed()` reporting.
@JakeWharton
Copy link
Copy Markdown
Contributor Author

We could optionally make the Handler reference nullable and volatile and set it to null to save the boolean.

@artem-zinnatullin
Copy link
Copy Markdown
Contributor

Hm, I'm not sure that reordering is a good idea, Handler.removeCallback takes much more time than writing to a volatile field. I think we want to make disposed change visible asap. It's not harmful though, as Handler.removeCallback is going to noop on subsequent calls.

Also I'm pretty sure it's still possible to see disposed == true in run() using code from referenced issue.
There is a race between executing ScheduledRunnable on main thread and dispose() from io thread.

I have an idea on how to not do dispose on upstream thread which should remove the race in first place for referenced issue, but it requires an additional field and I haven't tested it yet, so I might be wrong.

// I'm on mobile, can elaborate later

@JakeWharton
Copy link
Copy Markdown
Contributor Author

It takes time because it's the thing that's actual doing the cancelation. The boolean is redundant information solely for internal tracking purposes.

@JakeWharton
Copy link
Copy Markdown
Contributor Author

Moreover, setting the boolean before removeCallbacks completes can be argued as a bug as you can observe isDisposed() true on another thread before the lock on the message queue is taken.

@JakeWharton
Copy link
Copy Markdown
Contributor Author

Ah, I realize what you're likely going to post as I don't think I was clear. It's always the case that the boolean can be observed to be set while the runnable is running when they run concurrently. What this change does, however, is eliminate the ability for the boolean to reflect a state that's different than the real underlying race. That is, either Handler.removeCallbacks or MessageQueue.next is going to win and that becomes the decision to run or not. If you are disposing concurrently there is always a race. If you are disposing on the same looper as the handler then you have no problems. The fact that RxJava causes a race where you otherwise don't expect one is not this library's problem as it affects all schedulers.

@JakeWharton JakeWharton merged commit e09c2d7 into 2.x Jul 1, 2018
@JakeWharton JakeWharton deleted the race branch July 1, 2018 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants