fix(16033) - Fix data race in BlockingFloatHeap.poll function#16207
fix(16033) - Fix data race in BlockingFloatHeap.poll function#16207vijaykriishna wants to merge 1 commit into
Conversation
vijaykriishna
force-pushed
the
fix/16033-10x
branch
2 times, most recently
from
d259c24 to
35ecd55
Compare
| * GITHUB#16172: Bulk-evaluate skip-indexed sorted and sorted-set doc-values range queries via a block-aware | ||
| DocValuesRangeIterator#intoBitSet. (Costin Leau) | ||
|
|
||
| * GITHUB#16033: Fix data race in BlockingFloatHeap.poll function. (Vijay) |
There was a problem hiding this comment.
shouldn't this be under bug fixes?
There was a problem hiding this comment.
shouldn't this be under bug fixes?
You're right. Placed it under bug fixes section.
| if (heap.size() > 0) { | ||
| heap.poll(); | ||
| } | ||
| } |
There was a problem hiding this comment.
checking heap.size() > 0 from one thread and then calling heap.poll() from that same thread, but another thread can poll() between those two calls. this is the exact same TOCTOU thing that you are fixing here :)
can we either catch IllegalStateException from poll() as an expected outcome, or can wrap the check-then-poll in a try/catch?
There was a problem hiding this comment.
checking heap.size() > 0 from one thread and then calling heap.poll() from that same thread, but another thread can poll() between those two calls. this is the exact same TOCTOU thing that you are fixing here :)
can we either catch IllegalStateException from poll() as an expected outcome, or can wrap the check-then-poll in a try/catch?
You're right. heap.size check does exists in poll function. Also, IllegalStateException is handled implicitly. Cleaned up the code.
| negativeDetected.incrementAndGet(); | ||
| } | ||
| if (size > 0) { | ||
| heap.poll(); |
There was a problem hiding this comment.
same as above..should catch IllegalStateException from poll() as an expected outcome, or wrap the check-then-poll in a try/catch.
| // Continue | ||
| } | ||
| for (int i = 0; i < operationsPerProducer; i++) { | ||
| if (heap.size() > 0) { |
There was a problem hiding this comment.
same as above..should catch IllegalStateException from poll() as an expected outcome, or wrap the check-then-poll in a try/catch.
| barrier.await(); | ||
| for (int j = 0; j < 25; j++) { | ||
| if (heap.size() > 0) { | ||
| heap.poll(); |
There was a problem hiding this comment.
same as above..should catch IllegalStateException from poll() as an expected outcome, or wrap the check-then-poll in a try/catch.
| /** | ||
| * Unit tests for {@link BlockingFloatHeap} with focus on thread-safety and data race conditions. | ||
| */ | ||
| public class TestBlockingFloatHeap extends LuceneTestCase { |
There was a problem hiding this comment.
I see more things here, which doesn't test this bug except testDataRaceOnPoll...other things i dont think other tests fail without the fix, we can keep testDataRaceOnPoll and remove others?
There was a problem hiding this comment.
I see more things here, which doesn't test this bug except testDataRaceOnPoll...other things i dont think other tests fail without the fix, we can keep testDataRaceOnPoll and remove others?
You're right. testDataRaceOnPoll() is the only test that specifically reproduces the race fixed in this change. The other tests are broader concurrency checks and don't appear to fail without the fix. I've removed them and kept only testDataRaceOnPoll().
| }); | ||
|
|
||
| Thread peekThread = | ||
| new Thread( |
There was a problem hiding this comment.
peek i don't see is being the fix? guess this test is outside the scope..
vijaykriishna
force-pushed
the
fix/16033-10x
branch
from
35ecd55 to
65825c1
Compare
vijaykriishna
force-pushed
the
fix/16033-10x
branch
from
65825c1 to
34500fc
Compare
2 participants
Description
The data race in the poll() method has been resolved by moving the lock acquisition to before the if (size > 0) check. This ensures that the entire operation—both the validation and the heap operations—is protected by the lock as a single atomic unit.
Resolves: #16033