Add ThreadPolicy.ioBound for blocking consumers, and fix BatchQueue shutdown ordering - #13979
Merged
Merged
Conversation
wu-sheng
force-pushed
the
batchqueue/io-bound-thread-policy
branch
from
August 14, 2026 12:57
0f43dcd to
54a1be4
Compare
wu-sheng
force-pushed
the
batchqueue/io-bound-thread-policy
branch
2 times, most recently
from
August 14, 2026 14:57
bbbe752 to
c510b55
Compare
`ioBound(N)` declares that a queue's consumers spend most of their time blocked, typically on I/O. Such a queue runs its drain loops on virtual threads where the runtime provides them (JDK 25+, via VirtualThreads.createScheduledExecutor) and on N platform threads otherwise. The count is identical on both paths, so concurrency, batching, back-pressure, drop semantics and per-partition ordering are unaffected by the fallback — only the thread substrate changes, plus shutdown latency, since the platform scheduler can drop drain tasks parked on their idle backoff and the virtual adapter cannot. The purpose is narrow: avoid paying for N parked OS threads. It does not raise concurrency, because the drain loop is the task processor and nothing is handed off — consumer concurrency is min(threadCount, partitionCount) either way, so an IO-bound queue wants threads and partitions 1:1 with a small per-partition buffer. There is deliberately no CPU-proportional form and no cpuCores(...).ioBound(): virtual threads are not preemptive, so CPU-bound work would hold its carrier and starve the pool shared with GRPCServer and HTTPServer. L1/L2/TopN stay on cpuCores/fixed. No shipped queue uses the new policy. shutdown() separately had a pre-existing race: it ran the final drain on the caller's thread while drain loops could still be running, invoking a handler from two threads and breaking the single-drain-thread invariant workers such as MetricsAggregateWorker document. It now cancels the periodic rebalance task — which does not read `running`, and whose loop on a virtual-thread scheduler exits only on interrupt — shuts the scheduler down, waits for orderly exit, and takes an exclusive dispatch lock before draining. That lock, not the wait, is the guarantee: awaitTermination is a courtesy that may time out or be interrupted. Drain loops hold the read lock for the whole cycle — the running recheck, the dequeue, notifyIdle() and the dispatch. All four are needed: an unlocked dequeue lets shutdown dispatch a newer batch ahead of one a task already holds, so the older batch lands after shutdown() returned; notifyIdle() outside the lock runs onIdle() concurrently with the final dispatch, and MetricsAggregateWorker flushes the same worker state from onIdle(); and the loop's running test can pass just as shutdown completes. Read locks are shared, so drain loops still dispatch concurrently with one another. A consumer is never interrupted mid-batch, so no truncated executeBatch or stream send. Callers that lose the shutdown CAS await the winner's completion latch, unbounded — the winner may spend all of shutdownTimeoutMs in awaitTermination and then block on the write lock for as long as a consumer runs, so any fixed bound would let a loser return while the queue is still draining. The platform scheduler also sets setExecuteExistingDelayedTasksAfterShutdownPolicy(false) so drain tasks on their idle backoff do not hold up termination; interrupts are deferred and restored only after dispatch returns. Also fixes a long-standing test bug that has kept master's CI red. Four oal-rt tests register scopes in @BeforeAll with a single try around the whole block. notify() throws when a scope id is already registered, so once any sibling has registered even one scope, the first call throws and every scope after it is silently skipped — the tests then fail at use time with "ScopeDefine name = ... not found". OALSourceAttributionTest registers Service alone, so whenever surefire's file-system order puts it first, OALClassGeneratorV2Test loses Endpoint, ServiceRelation, K8SServiceInstance and TCPService. That order occurs on the CI runners but not on macOS, which is why it reproduces only in CI. Reproduced locally with -Dsurefire.runOrder=reversealphabetical, and each registration is now guarded independently. The module passes under filesystem, alphabetical and reversealphabetical ordering.
wu-sheng
force-pushed
the
batchqueue/io-bound-thread-policy
branch
from
August 14, 2026 15:22
c510b55 to
f216c11
Compare
mrproliu
approved these changes
Aug 15, 2026
2 tasks
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add
ThreadPolicy.ioBound(N)tolibrary-batch-queue, and fixBatchQueue.shutdown()orderingIf this is non-trivial feature, paste the links/URLs to the design doc. — no separate design doc; the rationale is in
library-batch-queue/CLAUDE.md.Update the documentation to include this new feature. —
library-batch-queue/CLAUDE.mdgains anioBoundsection and aShutdown ordersection. No user-facing docs change: nothing underdocs/references BatchQueue except the changelogs, and no shipped queue adoptsioBound, so the thread-name table inchanges-10.4.0.mdstays accurate.Tests(including UT, IT, E2E) are added to verify the new feature. — 5 new
ThreadPolicytests, 4 new shutdown tests. 76 pass in the module.If it's UI related, attach the screenshots below. — n/a
If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
Update the
CHANGESlog.1.
ThreadPolicy.ioBound(N)— newDeclares that a queue's consumers spend most of their time blocked, typically on I/O. Such a queue runs its drain loops on virtual threads where the runtime provides them (JDK 25+, via
VirtualThreads.createScheduledExecutor) and on N platform threads otherwise.The count is identical on both paths, so concurrency, batching, back-pressure, drop semantics and per-partition ordering are unaffected by the fallback — only the thread substrate changes. The purpose is narrow: avoid paying for N parked OS threads. It does not raise concurrency, because the drain loop is the task processor and nothing is handed off — consumer concurrency is
min(threadCount, partitionCount)on both paths. An IO-bound queue therefore wants threads and partitions 1:1 with a small per-partition buffer.There is deliberately no CPU-proportional form and no
cpuCores(...).ioBound(): virtual threads are not preemptive, so a CPU-only task holds its carrier to completion and would starve the pool shared withGRPCServerandHTTPServer. The API cannot express that combination. L1/L2/TopN stay oncpuCores/fixed.No existing queue uses the new policy, so this branch is inert until something opts in.
2.
shutdown()ordering — pre-existing bugshutdown()ran its final drain on the caller's thread while drain loops could still be insideconsume(), invoking the same handler from two threads and breaking the single-drain-thread invariant that workers such asMetricsAggregateWorkerdocument (its own lines 53-54). New order:The order is load-bearing: the rebalance task must be cancelled before
awaitTermination(on a virtual-thread scheduler its loop exits only on interrupt, andshutdown()does not interrupt), andrunning = falsemust precede the cancel because the rebalance fence exits viarunning, not the interrupt —LockSupport.parkNanosreturns on interrupt without throwing.The platform scheduler now also sets
setExecuteExistingDelayedTasksAfterShutdownPolicy(false)so drain tasks parked on their idle backoff do not hold up termination.On timeout the queue logs and proceeds rather than calling
shutdownNow(): no consumer is written to tolerate interruption mid-batch, and a truncatedexecuteBatchor stream send is worse than the race being fixed.shutdownTimeoutMsis per-queue (default 500ms).Severity in practice is latent rather than live.
shutdownAll()has no production callers; the only live path isGRPCRemoteClient.close(), whose consumer creates a freshStreamObserverper batch and tolerates the concurrent call. The queues that would not tolerate it are never shut down. Correct by coincidence on both sides.3. Fix a long-standing
oal-rttest bug that has kept master's CI redFour tests register scopes in
@BeforeAllwith a singletryaround the whole block:notify()throwsUnexpectedExceptionwhen a scope id is already registered, so once any siblinghas registered even one scope, the first call throws and every scope after it is silently
skipped. The tests then fail at use time with
ScopeDefine name = ... not found.OALSourceAttributionTestregistersServicealone. Whenever surefire's file-system run orderputs it first,
OALClassGeneratorV2TestlosesEndpoint,ServiceRelation,K8SServiceInstanceand
TCPService— exactly the four scopes CI reports, in exactly the four failing test methods.That ordering occurs on the CI runners but not on macOS, which is why it reproduces only in CI.
Reproduced locally with
-Dsurefire.runOrder=reversealphabetical(5 tests, 4 errors, identicalmessages). Each registration is now guarded independently, and the module passes under
filesystem,alphabeticalandreversealphabetical.This is a pre-existing bug, not one this branch introduced — master's own scheduled runs
(
102af09,dae21ce,a2f1f16,a2498aa) fail the same way. It is folded in here becausefail-fast cancels the rest of the matrix, so nothing else can go green until it is fixed.
Reviewer notes
ThreadPolicy.ioBoundtakes the virtual branch there rather than the platform fallback.(
testLosingShutdownCallerAwaitsCompletion).Each guard was verified by reverting the production change and confirming the corresponding test
fails:
expected: <1> but was: <2>for concurrent dispatch, on both the ordering fix and thetimeout path, and
expected: <0> but was: <1>for a losing shutdown caller returning early.Out of scope
No change to any existing queue's configuration. Every current caller uses a non-
ioBoundpolicy,so that branch is dead until something opts in.
The periodic rebalancer deliberately shares the drain pool rather than having its own thread: L1 is
cpuCores(1.0), one thread per core, and a dedicated rebalance thread would make it N+1 threads onN cores — the oversubscription that costs context switches and cache reloads. Sharing also means
rebalancing is naturally deferred while all drain loops are busy, which is the right moment not to
be reshuffling partition ownership. This PR leaves that as is.