[common] Fix permit accounting of SemaphoredDelegatingExecutor under interruption and rejection - #9511
Conversation
…interruption and rejection execute() restored the interrupt flag and then submitted the task anyway, so the wrapper released a permit that was never acquired and availablePermits() climbed above permitCount. It now throws RejectedExecutionException, the Executor-contract signal that the task will not run, so a caller that handed the task to a CompletableFuture stage unwinds instead of waiting forever. All four submit/execute paths now release the acquired permit when the delegate rejects the task, through a release-once guard on the wrapper: a delegate that runs the task inline can both run the wrapper and let the task's own RejectedExecutionException out of the same call, and releasing twice there would inflate the count the same way. Assisted-by: GLM-5.3
…ounting The previous commit made execute() throw RejectedExecutionException when the permit acquire was interrupted. That is broader than intended: Semaphore.acquire goes through AQS acquireSharedInterruptibly, which throws as soon as the caller carries an interrupt flag, even with every permit free. So any execute() call from a thread that is already interrupted, which is what a Flink or Spark task looks like while it is being cancelled, stopped submitting its task and threw instead. Verified with a probe: permitCount 2, both permits free, interrupt flag set, and execute() threw while the task never ran. Keep the original behavior instead. An interrupted submitter restores the flag and hands the task to the delegate as before; only the accounting changes, by recording that no permit backs that task so its wrapper does not hand back a permit nobody acquired. The release-once flag now carries that state directly: permitHeld starts false when the acquire failed. Assisted-by: GLM-5.3
|
Pushed 28a2d67 after the first version of this PR timed out the The problem was mine and it was wider than the commit message claimed. The new commit keeps the original behavior: an interrupted submitter restores the flag and hands the task to the delegate as before, and only the accounting changes, by recording that no permit backs that task so its wrapper does not hand back a permit nobody acquired. That job is now green in 42m45s, with The one remaining red check, |
|
+1 |
|
Thank you @JingsongLi |
Purpose
close #9508
SemaphoredDelegatingExecutorbounds concurrency on a delegate pool with one semaphore permit per submitted task, released by the per-task wrapper when the task finishes. Two paths broke that accounting, and this PR fixes only the accounting: no caller sees a behavior change.execute()caughtInterruptedExceptionfrom the permit wait, restored the interrupt flag, and then submitted the task anyway. The wrapper released a permit that was never acquired, sogetAvailablePermits()climbed abovepermitCountand the ceiling this class exists to enforce was raised with nothing logged. The task still has to run:execute()has no failed-future channel, and the callers that reach this class throughCompletableFuture.supplyAsync(task, executor)would otherwise wait on a stage that never completes. So it still runs, and the wrapper now knows that no permit backs it and does not hand one back.Worth spelling out why the fix does not simply reject the task, since that was this PR's first version.
Semaphore.acquire()goes through AQSacquireSharedInterruptibly, which throws as soon as the caller carries an interrupt flag, even with every permit free. Rejecting onInterruptedExceptiontherefore does not mean "interrupted while waiting for a permit"; it means "anyexecute()from an already-interrupted thread", which is exactly what a Flink or Spark task looks like while it is being cancelled. A probe withpermitCount2, both permits free and the flag set showedexecute()throwing while the task never ran.All four submit/execute paths now return the acquired permit when the delegate rejects the task, which previously leaked it, because the wrapper that would have released it never ran. The release goes through a release-once flag on the wrapper rather than an unconditional
release()at the call site, for two reasons: a delegate that runs the task inline (CallerRunsPolicyon a saturated bounded pool, or any direct executor) can both run the wrapper and let aRejectedExecutionExceptionthrown by the task itself out of the same call, and the same flag records whether a permit was ever acquired for this task at all.Scope note: every current construction site (
FileOperationThreadPool,ManifestReadThreadPool,GlobalIndexReadThreadPool,CatalogSplitEnumerator) wraps a process-wide static pool with an unbounded queue and the defaultAbortPolicy, so the interrupt path is the one reachable in production today. The catch stays narrowed toRejectedExecutionException, the contract signal for "the delegate will not take this task"; other throwables out of the delegate are left alone, and the release-once flag makes widening that catch safe later if a bounded or custom delegate ever needs it.Tests
SemaphoredDelegatingExecutorTestis new. The class had no test before.testInterruptedExecuteRunsTaskWithoutInflatingPermits: waits until the submitter is provably parked on a zero-permit semaphore, interrupts it once, then drains the delegate and asserts the task ran, the interrupt flag survived, and the permit count is unchanged rather than one higher.testExecuteWithInterruptFlagAlreadySetKeepsPermitCount: the same invariants for a submitter that carries the flag before it callsexecute()with both permits free, which is the case that made the first version of this PR reject work during cancellation.testRejectedByDelegateReleasesPermit: a shut-down delegate, asserting the permit returns to 1 after each of the four entry points (execute,submit(Callable),submit(Runnable),submit(Runnable, result)).testInlineExecutionReleasesPermitOnlyOnce: aThreadPoolExecutorwithcorePoolSize1, a queue of 1 andCallerRunsPolicy, saturated so the wrapper runs in the calling thread while the task's ownRejectedExecutionExceptionescapesexecute(); asserts the permit count is 1, not 2.testNormalExecutionKeepsPermitsBalanced: five tasks over two permits, asserting all five ran and the count is back to two after the delegate terminates. This one also passes without the fix; it is the first pin on the normal release path, which had none.Three of the five fail against master: both interrupt tests on the inflated count (1 instead of 0, and 3 instead of 2) and the rejection test on the leaked permit.
mvn -pl paimon-common clean teston JDK 8: 12470 tests, 0 failures, 0 errors.SinkSavepointITCase, the test the earlier version of this PR left running until the CI job timeout, passes locally 3 for 3 with this version. checkstyle, spotless, enforcer and rat run clean.