-
Notifications
You must be signed in to change notification settings - Fork 107
fix: Fix race condition in BatcherImpl flush #1200
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1200 +/- ##
=========================================
Coverage 79.06% 79.07%
- Complexity 1197 1198 +1
=========================================
Files 205 205
Lines 5268 5270 +2
Branches 435 436 +1
=========================================
+ Hits 4165 4167 +2
Misses 930 930
Partials 173 173
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vam PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but how painful will it be to add a test for that?
I think a test would be quite difficult, but I can try in a separate PR. This is currently a blocker for a customer, so I'd prefer to get this out asap. Would it be possible to cut a release with this change either today or tomorrow? |
Added a stress test..wasnt as bad as I expected |
35fe8fb
to
1ec3428
Compare
Currently the following race condition exists: T1 - awaitAllOutstandingBatches checks that numOfOutstandingBatches > 0 T2 - onBatchCompletion decrements numOfOutstandingBatches T2 - flushLock.notifyAll() T1 - flushLock.wait() so T1 will wait indefinitely The fix is quite simple: make sure that the there batches to wait for after acquiring the lock
1ec3428
to
ebad62f
Compare
Currently the following race condition exists:
T1 - awaitAllOutstandingBatches checks that numOfOutstandingBatches > 0
T2 - onBatchCompletion decrements numOfOutstandingBatches
T2 - flushLock.notifyAll()
T1 - flushLock.wait()
so T1 will wait indefinitely
The fix is quite simple: make sure that the there batches to wait for after acquiring the lock