FrankChen021 commented on code in PR #20460:
URL: https://github.com/apache/druid/pull/20460#discussion_r4155284062
##########
processing/src/main/java/org/apache/druid/frame/processor/FrameProcessorExecutor.java:
##########
@@ -666,13 +703,26 @@ private void cancel(final Set<FrameProcessor<?>>
processorsToCancel)
}
}
- private <T> void registerCancelableProcessor(final FrameProcessor<T>
processor, @Nullable final String cancellationId)
+ /**
+ * Register a processor so {@link #cancel(String)} can find it.
+ *
+ * @return true if registered, or if there is no cancellationId to register
under. False if {@code cancellationId}
+ * is already canceled, in which case the caller must not run the processor
and owns cleaning it up.
+ */
+ private <T> boolean registerCancelableProcessor(
+ final FrameProcessor<T> processor,
+ @Nullable final String cancellationId
+ )
{
if (cancellationId != null) {
synchronized (lock) {
+ if (!activeCancellationIds.contains(cancellationId)) {
Review Comment:
[P2] Do not admit a canceled run into a reused cancellation ID
**Finding:** This check only tests whether the string is currently active,
so a run paused after its return future was registered can be canceled,
followed by registerCancellationId(cancellationId), before it reaches this
block. The old run then sees the new registration, inserts its already-canceled
processor, and executes it; because its canceled future listener already ran,
no later cancellation is guaranteed to remove or clean that processor, allowing
canceled work and resources to survive under the new generation.
**Suggestion:** Bind processor registration to the same cancellation
generation as the return future (or reject it when that future is already
canceled), and add a cancel-then-re-register race test for the same ID.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
