FrankChen021 commented on code in PR #20074:
URL: https://github.com/apache/druid/pull/20074#discussion_r3821787432
##########
processing/src/main/java/org/apache/druid/common/asyncresource/SettableAsyncResource.java:
##########
@@ -263,9 +268,11 @@ public void close()
default -> throw DruidException.defensive("Already closed");
};
- // Clear result and canceler to allow GC.
+ // Clear result and canceler to allow GC. Dropping the pending ready
callbacks ensures they are not fired if
+ // close() was called before set/setException.
result = null;
canceler = null;
+ readyCallbacks.clear();
Review Comment:
Reviewed 3 of 3 changed files.
The Javadocs clarify the intended usage model, but the lifecycle race
remains: await() relies on a callback that close() now removes. An independent
cancellation thread can therefore strand a single consumer in await()
indefinitely. Please add a close-aware wake-up path and regression test.
<!-- mergelens:review -->
##########
processing/src/main/java/org/apache/druid/common/asyncresource/SettableAsyncResource.java:
##########
@@ -308,6 +315,17 @@ private boolean setInternal(Either<Throwable,
ResourceHolder<T>> value)
canceler = null;
callbacksToFire = drainCallbacks();
}
+
+ if (!didSet && value.isError()) {
+ // Nothing will ever surface this error: get() on a closed resource
throws "Closed" and the callbacks are gone,
+ // so log it rather than let a failure that lost a race with close()
vanish
+ if (value.error() instanceof CancellationException) {
+ LOG.debug(value.error(), "Resource canceled by close().");
+ } else {
+ LOG.warn(value.error(), "Resource failed after close().");
Review Comment:
Reviewed 3 of 3 changed files.
The move to debug-level logging resolves this concern; late failures after
close will no longer pollute normal logs. No further action is needed for this
thread.
<!-- mergelens:review -->
--
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]
