Uh oh!
There was an error while loading. Please reload this page.
[fix](be) Fix waiter accounting in blocking queue - #65827
Merged
Conversation
jacktengg
commented
Jul 20, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jul 20, 2026
Contributor
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
hello-stephen
commented
Jul 20, 2026
Contributor
TPC-H: Total hot run time: 29322 ms |
hello-stephen
commented
Jul 20, 2026
Contributor
TPC-DS: Total hot run time: 176978 ms |
hello-stephen
commented
Jul 20, 2026
Contributor
ClickBench: Total hot run time: 25.14 s |
hello-stephen
commented
Jul 20, 2026
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jul 20, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
jacktenggforce-pushed
the
CIR-20956
branch
3 times, most recently
from
July 21, 2026 10:47
0fae3ac to
573ac08CompareIssue Number: None Related PR: apache#51596 Problem Summary: BlockingQueue split ownership of waiter counters between waiting threads and notifying threads. Around a timeout race, both sides could decrement the same registration, while a spurious wakeup could leave a stale registration behind. Once the counters drifted, a real waiter could be represented by zero and miss a notification, leaving routine load queue operations blocked until the one-hour fallback timeout. This is a missed wakeup and potentially long stall, rather than a permanent deadlock. In addition, try_put read the shutdown flag and queue size before acquiring the mutex, which introduced a data race. The following sequence shows how lock contention after a timeout corrupts the consumer waiter count: | Time | Consumer threads | Producer threads | get_waiting | Result | | --- | --- | --- | ---: | --- | | T0 | C increments the counter and calls wait_for, which releases the mutex. | - | 0 -> 1 | C is registered as a condition-variable waiter. | | T1 | C's timeout expires. C leaves the condition-variable wait set, but wait_for must reacquire the mutex before returning. | P acquires the mutex before C can reacquire it. | 1 | C's registration is still visible because C cannot update it without the mutex. | | T2 | C remains blocked while trying to reacquire the mutex. | P pushes an item, decrements the counter from 1 to 0, unlocks, and calls notify_one. | 1 -> 0 | The notification cannot wake C because its timed wait has already expired. | | T3 | C reacquires the mutex. wait_for returns timeout, so the old timeout branch decrements the same registration again. C then consumes the item. | - | 0 -> SIZE_MAX | The unsigned waiter count wraps around. | | T4 | After the queue becomes empty, a new consumer C2 increments the counter and starts waiting. | - | SIZE_MAX -> 0 | A real waiter is now represented by zero. | | T5 | Absent a spurious wakeup, shutdown, or another notification, C2 remains asleep until its next timeout, which is one hour by default. | P2 pushes an item, observes zero, and skips notify_one. | 0 | C2 suffers a missed wakeup and a potentially one-hour stall. | Make each waiting thread register immediately before wait_for and always unregister after wait_for returns, regardless of the wakeup reason. Notifiers now inspect waiter counters under the mutex without consuming registrations, then release the mutex before notifying. Remove the unsynchronized try_put fast path and add deterministic tests for both consumer and producer timeout races. Fix potential long stalls in backend blocking queues under concurrent timeout and notification. - Test: Unit Test - Added BlockingQueueWaiterTest.TimedGetNotificationRace and BlockingQueueWaiterTest.TimedPutNotificationRace. - `./run-be-ut.sh --run --filter='BlockingQueueWaiterTest.*' -j 32` - Behavior changed: Yes. Waiter registrations are now released by the waiting thread, and try_put checks queue state only while holding the mutex. - Does this need documentation: No
jacktengg
commented
Jul 21, 2026
ContributorAuthor
/review |
Contributor
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 25th, 2026 6:03 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
jacktengg
commented
Jul 21, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jul 21, 2026
Contributor
TPC-H: Total hot run time: 29116 ms |
hello-stephen
commented
Jul 21, 2026
Contributor
TPC-DS: Total hot run time: 177075 ms |
hello-stephen
commented
Jul 21, 2026
Contributor
ClickBench: Total hot run time: 24.86 s |
hello-stephen
commented
Jul 21, 2026
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jul 21, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
PR approved by at least one committer and no changes requested. |
Contributor
PR approved by anyone and no changes requested. |
HappenLee
commented
Jul 22, 2026
Contributor
/review |
Contributor
There was a problem hiding this comment.
Review opinion: approve. I found no blocking correctness issues in the current head.
Critical checkpoint conclusions:
- Goal and proof: The change fixes split waiter-counter ownership and the unlocked
try_putreads. Each waiter now owns one registration perwait_for()epoch, and the symmetric get/put tests force the timeout-versus-mutex-reacquisition ordering that previously wrapped the counters before verifying that the next real waiter is notified promptly. - Scope: The production change is localized to
BlockingQueue; test hooks and counter accessors are restricted toBE_TEST. The larger test diff is focused on deterministic reproduction and cleanup. - Concurrency: Producer and consumer threads share
_shutdown, the list, and both waiter counters under the single_lock; wait-time totals remain atomic. Predicate changes and waiter snapshots occur under that mutex, while notification occurs after unlock. Timeout, notification, spurious wakeup, and shutdown each cause exactly one waiter-side decrement, with no nested production locks or lock-order cycle. - Lifecycle and static initialization:
shutdown()publishes_shutdownunder_lockand then wakes both condition variables; awakened getters may drain queued data and putters fail as before. The test cleanup releases blocked callbacks, shuts down the queue, and joins every worker before promises, queues, coordinators, or callback captures are destroyed. No new cross-translation-unit static dependency is introduced. - Configuration: No configuration item is added or changed.
- Compatibility: No wire protocol, persisted format, function symbol, storage version, or rolling-upgrade contract changes.
- Parallel paths:
blocking_get,blocking_put, andtry_putuse the same corrected notification rule. Routine-load producers/consumers and FIFO work pools were traced.BlockingPriorityQueuehas different notifier-owned accounting that can over-count but cannot reproduce this underflow/missed-wakeup failure, so it does not require this change. - Special conditions: The
has_*_waitersnapshots are taken under_lock. A false snapshot cannot miss an already registered waiter, and a later waiter observes the already-changed item/slot predicate instead of sleeping. - Test coverage: The two new BE unit tests cover both consumer and producer timeout races, verify counter state, distinguish notification from the 30-second fallback using a five-second deadline, and have bounded fatal-assertion cleanup. Existing tests cover basic FIFO, shutdown/drain behavior, and multiple producers/consumers.
- Test results: The PR's compile, Linux BE UT, format, and coverage checks succeeded. The macOS BE UT job failed before compilation because that runner exposed JDK 25 while the build requires JDK 17; this is unrelated to the patch. Per the review-runner contract, I did not run a local build.
- Observability: Existing queue wait-time counters remain intact; this internal synchronization correction does not need a new log or metric.
- Transactions, persistence, and data writes: Not applicable; no transaction state, persisted metadata, visibility version, delete bitmap, or data-write atomicity is changed.
- FE/BE propagation: No new variable or protocol field crosses process boundaries.
- Performance: Removing the racy
try_putfast path adds the required mutex acquisition for failed attempts. Successful paths keep queue mutation under the mutex and notify after unlock, avoiding wake-up contention; no new production allocation or scan is introduced. - Other review checks: Error behavior is unchanged, no
Statusis dropped, memory ownership is unchanged, and no additional issue was found after the final changed-file and unresolved-risk sweep.
User focus: No additional focus points were provided; the entire two-file PR was reviewed.
Uh oh!
There was an error while loading. Please reload this page.
github-actionsBot
pushed a commit
that referenced
this pull request
Jul 23, 2026
### What problem does this PR solve? Issue Number: None Problem Summary: BlockingQueue split ownership of waiter counters between waiting threads and notifying threads. Around a timeout race, both sides could decrement the same registration, while a spurious wakeup could leave a stale registration behind. Once the counters drifted, a real waiter could be represented by zero and miss a notification, leaving routine load queue operations blocked until the one-hour fallback timeout. This is a missed wakeup and potentially long stall, rather than a permanent deadlock. In addition, try_put read the shutdown flag and queue size before acquiring the mutex, which introduced a data race. The following sequence shows how lock contention after a timeout corrupts the consumer waiter count: | Time | Consumer threads | Producer threads | get_waiting | Result | | --- | --- | --- | ---: | --- | | T0 | C increments the counter and calls wait_for, which releases the mutex. | - | 0 -> 1 | C is registered as a condition-variable waiter. | | T1 | C's timeout expires. C leaves the condition-variable wait set, but wait_for must reacquire the mutex before returning. | P acquires the mutex before C can reacquire it. | 1 | C's registration is still visible because C cannot update it without the mutex. | | T2 | C remains blocked while trying to reacquire the mutex. | P pushes an item, decrements the counter from 1 to 0, unlocks, and calls notify_one. | 1 -> 0 | The notification cannot wake C because its timed wait has already expired. | | T3 | C reacquires the mutex. wait_for returns timeout, so the old timeout branch decrements the same registration again. C then consumes the item. | - | 0 -> SIZE_MAX | The unsigned waiter count wraps around. | | T4 | After the queue becomes empty, a new consumer C2 increments the counter and starts waiting. | - | SIZE_MAX -> 0 | A real waiter is now represented by zero. | | T5 | Absent a spurious wakeup, shutdown, or another notification, C2 remains asleep until its next timeout, which is one hour by default. | P2 pushes an item, observes zero, and skips notify_one. | 0 | C2 suffers a missed wakeup and a potentially one-hour stall. | Make each waiting thread register immediately before wait_for and always unregister after wait_for returns, regardless of the wakeup reason. Notifiers now inspect waiter counters under the mutex without consuming registrations, then release the mutex before notifying. Remove the unsynchronized try_put fast path and add deterministic tests for both consumer and producer timeout races. ### Release note Fix potential long stalls in backend blocking queues under concurrent timeout and notification. ### Check List (For Author) - Test: Unit Test - Added BlockingQueueWaiterTest.TimedGetNotificationRace and BlockingQueueWaiterTest.TimedPutNotificationRace. - `./run-be-ut.sh --run --filter='BlockingQueueWaiterTest.*' -j 32` - Behavior changed: Yes. Waiter registrations are now released by the waiting thread, and try_put checks queue state only while holding the mutex. - Does this need documentation: No
yiguolei pushed a commit
that referenced
this pull request
Jul 23, 2026
0AyanamiRei pushed a commit
to 0AyanamiRei/doris
that referenced
this pull request
Jul 27, 2026
### What problem does this PR solve? Issue Number: None Problem Summary: BlockingQueue split ownership of waiter counters between waiting threads and notifying threads. Around a timeout race, both sides could decrement the same registration, while a spurious wakeup could leave a stale registration behind. Once the counters drifted, a real waiter could be represented by zero and miss a notification, leaving routine load queue operations blocked until the one-hour fallback timeout. This is a missed wakeup and potentially long stall, rather than a permanent deadlock. In addition, try_put read the shutdown flag and queue size before acquiring the mutex, which introduced a data race. The following sequence shows how lock contention after a timeout corrupts the consumer waiter count: | Time | Consumer threads | Producer threads | get_waiting | Result | | --- | --- | --- | ---: | --- | | T0 | C increments the counter and calls wait_for, which releases the mutex. | - | 0 -> 1 | C is registered as a condition-variable waiter. | | T1 | C's timeout expires. C leaves the condition-variable wait set, but wait_for must reacquire the mutex before returning. | P acquires the mutex before C can reacquire it. | 1 | C's registration is still visible because C cannot update it without the mutex. | | T2 | C remains blocked while trying to reacquire the mutex. | P pushes an item, decrements the counter from 1 to 0, unlocks, and calls notify_one. | 1 -> 0 | The notification cannot wake C because its timed wait has already expired. | | T3 | C reacquires the mutex. wait_for returns timeout, so the old timeout branch decrements the same registration again. C then consumes the item. | - | 0 -> SIZE_MAX | The unsigned waiter count wraps around. | | T4 | After the queue becomes empty, a new consumer C2 increments the counter and starts waiting. | - | SIZE_MAX -> 0 | A real waiter is now represented by zero. | | T5 | Absent a spurious wakeup, shutdown, or another notification, C2 remains asleep until its next timeout, which is one hour by default. | P2 pushes an item, observes zero, and skips notify_one. | 0 | C2 suffers a missed wakeup and a potentially one-hour stall. | Make each waiting thread register immediately before wait_for and always unregister after wait_for returns, regardless of the wakeup reason. Notifiers now inspect waiter counters under the mutex without consuming registrations, then release the mutex before notifying. Remove the unsynchronized try_put fast path and add deterministic tests for both consumer and producer timeout races. ### Release note Fix potential long stalls in backend blocking queues under concurrent timeout and notification. ### Check List (For Author) - Test: Unit Test - Added BlockingQueueWaiterTest.TimedGetNotificationRace and BlockingQueueWaiterTest.TimedPutNotificationRace. - `./run-be-ut.sh --run --filter='BlockingQueueWaiterTest.*' -j 32` - Behavior changed: Yes. Waiter registrations are now released by the waiting thread, and try_put checks queue state only while holding the mutex. - Does this need documentation: No
wyxxxcat pushed a commit
to wyxxxcat/doris
that referenced
this pull request
Aug 17, 2026
### What problem does this PR solve? Issue Number: None Problem Summary: BlockingQueue split ownership of waiter counters between waiting threads and notifying threads. Around a timeout race, both sides could decrement the same registration, while a spurious wakeup could leave a stale registration behind. Once the counters drifted, a real waiter could be represented by zero and miss a notification, leaving routine load queue operations blocked until the one-hour fallback timeout. This is a missed wakeup and potentially long stall, rather than a permanent deadlock. In addition, try_put read the shutdown flag and queue size before acquiring the mutex, which introduced a data race. The following sequence shows how lock contention after a timeout corrupts the consumer waiter count: | Time | Consumer threads | Producer threads | get_waiting | Result | | --- | --- | --- | ---: | --- | | T0 | C increments the counter and calls wait_for, which releases the mutex. | - | 0 -> 1 | C is registered as a condition-variable waiter. | | T1 | C's timeout expires. C leaves the condition-variable wait set, but wait_for must reacquire the mutex before returning. | P acquires the mutex before C can reacquire it. | 1 | C's registration is still visible because C cannot update it without the mutex. | | T2 | C remains blocked while trying to reacquire the mutex. | P pushes an item, decrements the counter from 1 to 0, unlocks, and calls notify_one. | 1 -> 0 | The notification cannot wake C because its timed wait has already expired. | | T3 | C reacquires the mutex. wait_for returns timeout, so the old timeout branch decrements the same registration again. C then consumes the item. | - | 0 -> SIZE_MAX | The unsigned waiter count wraps around. | | T4 | After the queue becomes empty, a new consumer C2 increments the counter and starts waiting. | - | SIZE_MAX -> 0 | A real waiter is now represented by zero. | | T5 | Absent a spurious wakeup, shutdown, or another notification, C2 remains asleep until its next timeout, which is one hour by default. | P2 pushes an item, observes zero, and skips notify_one. | 0 | C2 suffers a missed wakeup and a potentially one-hour stall. | Make each waiting thread register immediately before wait_for and always unregister after wait_for returns, regardless of the wakeup reason. Notifiers now inspect waiter counters under the mutex without consuming registrations, then release the mutex before notifying. Remove the unsynchronized try_put fast path and add deterministic tests for both consumer and producer timeout races. ### Release note Fix potential long stalls in backend blocking queues under concurrent timeout and notification. ### Check List (For Author) - Test: Unit Test - Added BlockingQueueWaiterTest.TimedGetNotificationRace and BlockingQueueWaiterTest.TimedPutNotificationRace. - `./run-be-ut.sh --run --filter='BlockingQueueWaiterTest.*' -j 32` - Behavior changed: Yes. Waiter registrations are now released by the waiting thread, and try_put checks queue state only while holding the mutex. - Does this need documentation: No
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Problem Summary:
BlockingQueue split ownership of waiter counters between waiting threads and notifying threads. Around a timeout race, both sides could decrement the same registration, while a spurious wakeup could leave a stale registration behind. Once the counters drifted, a real waiter could be represented by zero and miss a notification, leaving routine load queue operations blocked until the one-hour fallback timeout. This is a missed wakeup and potentially long stall, rather than a permanent deadlock. In addition, try_put read the shutdown flag and queue size before acquiring the mutex, which introduced a data race.
The following sequence shows how lock contention after a timeout corrupts the consumer waiter count:
Make each waiting thread register immediately before wait_for and always unregister after wait_for returns, regardless of the wakeup reason. Notifiers now inspect waiter counters under the mutex without consuming registrations, then release the mutex before notifying. Remove the unsynchronized try_put fast path and add deterministic tests for both consumer and producer timeout races.
Release note
Fix potential long stalls in backend blocking queues under concurrent timeout and notification.
Check List (For Author)
./run-be-ut.sh --run --filter='BlockingQueueWaiterTest.*' -j 32