Notify a late-registering mailbox reader about buffered blocks - #19323
Notify a late-registering mailbox reader about buffered blocks#19323davecromberge wants to merge 1 commit into
Conversation
registerReader only assigned the reader. If a sender had already offered data or EOS, notifyReader() had found no reader and the wake-up was dropped, so the reader blocked until the query deadline. Reachable when a sender completes before the receiving stage registers, most easily on the in-memory path.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19323 +/- ##
=========================================
Coverage 67.12% 67.12%
Complexity 1424 1424
=========================================
Files 3462 3462
Lines 220677 220700 +23
Branches 35255 35259 +4
=========================================
+ Hits 148136 148154 +18
- Misses 60708 60728 +20
+ Partials 11833 11818 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
yashmayya
added
bug
yashmayya
left a comment
yashmayya
left a comment
There was a problem hiding this comment.
@davecromberge thanks for the PR. However, I tried to run the scenario with an agent and I do not think the root cause holds. Here's the evidence:
readBlockBlocking polls every mailbox one time before it waits on _newDataReady (BlockingMultiStreamConsumer.java:179), and the consumer registers all readers in its constructor (line 67). Registration always comes first, so that first poll finds any block that arrived before it. The lost wake-up costs nothing here.
I ran three scenarios through the real consumer on master, with a 3-second deadline:
- data and EOS before registration:
1 ms - EOS only before registration:
0 ms - two mailboxes, one finished before registration:
203 ms(200 mswas the slow sender)
No scenario waited to the deadline, so I do not think this PR corrects the count(*) timeout.
One path does drop a wake-up permanently. In state UPSTREAM_FINISHED, with _count at 0 and _pendingData more than 0, poll returns null (ReceivingMailbox.java:657). If the EOS is an error EOS, drainDataBlocks makes each parked sender return ALREADY_TERMINATED without a notifyReader call (line 628). The reader then waits to the deadline, and the query reports a timeout instead of the real error. With a full 1-slot queue and 6 parked senders, the reader waited to the deadline in 34 of 40 runs. The same test with a success EOS waited in 0 of 40 runs.
Can you share the evidence that ties your timeout to the registration order? A
No reader to notify log line appears in the benign case too, so this line does
not prove the fault.
|
You're right — thanks for digging into it. Registration always precedes the reader's first scan, so the dropped wake-up is redundant with that scan and this cannot be the cause of the timeout. Closing. Opened #19329 for the |
). Notify the mailbox reader when the last pending data offer gives up. An error EOS drains the queued data blocks and makes every parked sender give up; while those offers are still in flight `poll` returns null and asks the reader to wait for them, but a sender that gives up returns ALREADY_TERMINATED without notifying and the reader coalesces notifications into a single slot. Nothing wakes the reader again, so it blocks until the query deadline and the query reports EXECUTION_TIMEOUT instead of the real error. Replaces the previously carried cherry-pick of fix-contrib/mse-inmemory-mailbox-reader-race (9c6408a, upstream apache#19323), which is closed: that registerReader wake-up is provably unreachable because the reader scans every mailbox before it parks, so a notification dropped before registration is always redundant with that scan. Upstream PR: apache#19329 (open)
3 participants
ReceivingMailbox.registerReaderonly assigned the reader. If a sender had already offered data or EOS, the earliernotifyReader()found no reader and the wake-up was dropped, so the reader blocked until the query deadline and the query failed withEXECUTION_TIMEOUT(250).Reachable whenever a sender completes before the receiving stage registers — most easily on the in-memory path, where a leaf worker co-located with the consuming stage can finish in ~1ms. Observed in production on a
count(*)that otherwise takes single-digit ms.registerReadernow delivers the pending wake-up if blocks or EOS are already buffered, under the existing lock. Three tests cover data-before-registration, EOS-before-registration, and the empty-mailbox case (must not notify).