Fixes NPEs reported in ReactiveX#1702 by synchronizing queue. #2471
Conversation
…or regression.
| synchronized (queue) { | ||
| if (!queue.isEmpty()) { | ||
| TimedAction polled = queue.poll(); | ||
| polled.action.call(); |
There was a problem hiding this comment.
You should not call the action while holding the lock. Just do a synchronized poll and call the action if you got a non-null value.
In addition, I think the COUNTER_UPDATER and counter can be moved into the worker itself because it can ensure total order locally and we can avoid a global serialization point.
There was a problem hiding this comment.
Good call. I fixed the wrapping. Now I'm just using poll instead of isEmpty, since poll returns null iff isEmpty.
|
Almost good. Two things:
|
…worker class and ditches the separate field updater.
|
We are using AtomicFieldUpdaters to save on the instance cost of |
|
Done. On a related but side-note, do you know if these field updaters are proguard-safe for obfuscation? They reflect on field name for sure. It might be worth looking into. The fix would probably be implementing the abstract setters and getters, which would obviously be a bit more boilerplate and kinda less fun, but might be worth it if it reduces custom proguard rules required to use rx. |
|
Thanks for the changes, looks good to me. I don't know about ProGuard. |
Fixes NPEs reported in #1702 by synchronizing queue.
|
Looks like per the proguard manual, it does indeed recognize FieldUpdater declarations, so this should be fine. |
|
Wow I did not expect that. How uncharacteristically useful of them! Good to know for the future. |
|
http://proguard.sourceforge.net/manual/introduction.html See the 'Reflection' section (no anchor link). That lists the basic reflection methods it automatically detects. |
Also adds a unit test for regression.
It appears there is a potential race condition if something adds to/removes from the PQ while it's inside the 'poll' operation, which is where the exceptions in #1702 seem to have actually come from. Therefore, the initial null check didn't really address the original problem. The test here seems to reliably recreate those conditions.
I considered using a PriorityBlockingQueue instead of synchronized, but since the isEmpty and poll calls should not allow something to interleave between them and access the queue, a synchronized block seemed wiser here.