[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #10199: Shutdown Broker gracefully, but forcefully after brokerShutdownTimeoutMs

[GitBox <gi...@apache.org>]

michaeljmarshall commented on a change in pull request #10199:
URL: https://github.com/apache/pulsar/pull/10199#discussion_r612137732



##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/CompletableFutureCancellationHandler.java
##########
@@ -41,10 +41,16 @@
  * any "downstream" dependent futures. A cancellation or timeout that happens in any "upstream"
  * future will get handled.
  */
-class CompletableFutureCancellationHandler {
-    private volatile boolean cancelled;
+public class CompletableFutureCancellationHandler {
+    private enum CompletionStatus {
+        PENDING,
+        CANCELLED,
+        DONE
+    }
+    private volatile CompletionStatus completionStatus = CompletionStatus.PENDING;
     private volatile Runnable cancelAction;
     private final AtomicBoolean cancelHandled = new AtomicBoolean();
+    private boolean attached;

Review comment:
       Do you think it is worth adding the `volatile` keyword to `attached`? I can see that `attachToFuture` is synchronized, but if the method were called from two separate threads in close succession, I think it could lead to a race condition.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org