Skip to content

Commit 944bf24

Browse files
committed
fix: address second round of PR review comments
- Mark workerThread as volatile for cross-thread visibility - Remove unused imports (ManagedChannel, ManagedChannelBuilder) - Fail test explicitly when reflection fails instead of silently returning null - Assert interrupt status is preserved in startAndBlockExitsOnInterrupt Signed-off-by: Javier Aliaga <javier@diagrid.io>
1 parent e00a33e commit 944bf24

2 files changed

Lines changed: 11 additions & 9 deletions

File tree

durabletask-client/src/main/java/io/dapr/durabletask/DurableTaskGrpcWorker.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public final class DurableTaskGrpcWorker implements AutoCloseable {
6464
private final TaskHubSidecarServiceGrpc.TaskHubSidecarServiceBlockingStub sidecarClient;
6565
private final boolean isExecutorServiceManaged;
6666
private volatile boolean isNormalShutdown = false;
67-
private Thread workerThread;
67+
private volatile Thread workerThread;
6868

6969
DurableTaskGrpcWorker(DurableTaskGrpcWorkerBuilder builder) {
7070
this.orchestrationFactories = builder.orchestrationFactories;

durabletask-client/src/test/java/io/dapr/durabletask/DurableTaskGrpcWorkerShutdownTest.java

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,15 @@
1313

1414
package io.dapr.durabletask;
1515

16-
import io.grpc.ManagedChannel;
17-
import io.grpc.ManagedChannelBuilder;
1816
import org.junit.jupiter.api.Test;
1917

2018
import java.time.Duration;
2119
import java.time.Instant;
2220

2321
import static org.junit.jupiter.api.Assertions.assertFalse;
22+
import static org.junit.jupiter.api.Assertions.assertNotNull;
2423
import static org.junit.jupiter.api.Assertions.assertTrue;
24+
import static org.junit.jupiter.api.Assertions.fail;
2525

2626
/**
2727
* Unit tests for DurableTaskGrpcWorker shutdown behavior.
@@ -52,11 +52,10 @@ void workerThreadTerminatesPromptlyOnClose() throws Exception {
5252
// Wait for the worker thread to finish — the join is bounded so the
5353
// test doesn't hang if the fix regresses.
5454
Thread workerThread = getWorkerThread(worker);
55-
if (workerThread != null) {
56-
workerThread.join(Duration.ofSeconds(3).toMillis());
57-
assertFalse(workerThread.isAlive(),
58-
"Worker thread should have terminated after close()");
59-
}
55+
assertNotNull(workerThread, "Worker thread should be accessible via reflection");
56+
workerThread.join(Duration.ofSeconds(3).toMillis());
57+
assertFalse(workerThread.isAlive(),
58+
"Worker thread should have terminated after close()");
6059

6160
Duration elapsed = Duration.between(before, Instant.now());
6261
assertTrue(elapsed.toMillis() < 3000,
@@ -112,6 +111,8 @@ void startAndBlockExitsOnInterrupt() throws Exception {
112111

113112
assertFalse(blockingThread.isAlive(),
114113
"startAndBlock() thread should have exited after interrupt");
114+
assertTrue(blockingThread.isInterrupted(),
115+
"Interrupt status should be preserved after startAndBlock() exits");
115116

116117
worker.close();
117118
}
@@ -122,7 +123,8 @@ private Thread getWorkerThread(DurableTaskGrpcWorker worker) {
122123
f.setAccessible(true);
123124
return (Thread) f.get(worker);
124125
} catch (Exception e) {
125-
return null;
126+
fail("Failed to access workerThread field via reflection: " + e.getMessage());
127+
return null; // unreachable
126128
}
127129
}
128130
}

0 commit comments

Comments
 (0)