-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix TrampolineScheduler NullPointerException #1736
Conversation
I tried for about 30 minutes to replicate the NPE reported in ReactiveX#1702 but couldn't. It makes sense reading the code that an unsubscribe could trigger an NPE though so I'm fixing it as per recommendation of @DylanSale even though I can't replicate. I confirmed that the items are being put in the queue BEFORE the wip variable is incremented, so that concurrency seems okay.
I found it always odd to have a threadlocal queue and a per-scheduler wip counter. I can't test this now, but I think the NPE may happen if two tasks are scheduled from different threads on the same worker which makes wip = 2 but the thread that is consuming the tasks only sees its threadlocal queue with 1 item. |
It is indeed very odd, an artifact of early RxJava trying to match how Rx.Net did unsubscribes and scheduling. As of 0.17 when |
This was an awkward leftover from early version of RxJava (pre v0.17).
Just added another commit that removes the use of ThreadLocal. |
Fix TrampolineScheduler NullPointerException
Instead of removing it, I would have made |
It doesn't need to act like a ThreadLocal. It had a separate queue per Worker just like all Scheduler.Worker impls. Ben Christensen
|
I thought about it a bit and I was mistaken; the Worker needs to keep the tasks in order so threadlocal wip is not good. |
I tried for about 30 minutes to replicate the NPE reported in #1702 but couldn't.
It makes sense reading the code that an unsubscribe could trigger an NPE though so I'm fixing it as per recommendation of @DylanSale even though I can't replicate.
I confirmed that the items are being put in the queue BEFORE the wip variable is incremented, so that concurrency seems okay.