Skip to content

Commit 975eef4

Browse files
simplify test implementation
This version only checks the value of CompletableResultCode instead of trying to wait, it's simpler and also exposes the failure.
1 parent 6cd3de6 commit 975eef4

File tree

1 file changed

+17
-113
lines changed

1 file changed

+17
-113
lines changed

exporters/sender/okhttp/src/test/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSenderTest.java

Lines changed: 17 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,7 @@
1515
import java.io.IOException;
1616
import java.time.Duration;
1717
import java.util.Collections;
18-
import java.util.Locale;
1918
import java.util.Set;
20-
import java.util.logging.Logger;
21-
import java.util.stream.Collectors;
2219
import okhttp3.MediaType;
2320
import okhttp3.Protocol;
2421
import okhttp3.Request;
@@ -66,22 +63,21 @@ private static Response createResponse(int httpCode, String grpcStatus, String m
6663
}
6764

6865
@Test
69-
void shutdown_ShouldTerminateExecutorThreads() throws Exception {
70-
Logger logger = Logger.getLogger(OkHttpGrpcSenderTest.class.getName());
66+
void shutdown_CompletableResultCodeShouldWaitForThreads() throws Exception {
67+
// This test verifies that shutdown() returns a CompletableResultCode that only
68+
// completes AFTER threads terminate, not immediately.
7169

72-
// Create a sender that will try to connect to a non-existent endpoint
73-
// This ensures background threads are created
7470
OkHttpGrpcSender<TestMarshaler> sender =
7571
new OkHttpGrpcSender<>(
7672
"http://localhost:54321", // Non-existent endpoint
77-
null, // No compression
73+
null,
7874
Duration.ofSeconds(10).toNanos(),
7975
Duration.ofSeconds(10).toNanos(),
8076
Collections::emptyMap,
81-
null, // No retry policy
82-
null, // No SSL context
83-
null, // No trust manager
84-
null); // Use default executor (managed by sender)
77+
null,
78+
null,
79+
null,
80+
null);
8581

8682
// Send a request to trigger thread creation
8783
CompletableResultCode sendResult = new CompletableResultCode();
@@ -90,111 +86,19 @@ void shutdown_ShouldTerminateExecutorThreads() throws Exception {
9086
// Give threads time to start
9187
Thread.sleep(500);
9288

93-
// Capture OkHttp threads before shutdown
94-
Set<Thread> threadsBeforeShutdown = getOkHttpThreads();
95-
logger.info(
96-
"OkHttp threads before shutdown: "
97-
+ threadsBeforeShutdown.size()
98-
+ " threads: "
99-
+ threadsBeforeShutdown.stream()
100-
.map(Thread::getName)
101-
.collect(Collectors.joining(", ")));
102-
103-
// Verify threads exist
104-
assertFalse(
105-
threadsBeforeShutdown.isEmpty(), "Expected OkHttp threads to be present before shutdown");
106-
107-
// Call shutdown and wait for it to complete
10889
CompletableResultCode shutdownResult = sender.shutdown();
10990

110-
// Wait for shutdown to complete (this should succeed once the executor threads terminate)
91+
// The key test: the CompletableResultCode should NOT be done immediately
92+
// because we need to wait for threads to terminate.
93+
// With the old code (immediate ofSuccess()), this would fail.
94+
assertFalse(
95+
shutdownResult.isDone(),
96+
"CompletableResultCode should not be done immediately - it should wait for thread termination");
97+
98+
// Now wait for it to complete
11199
shutdownResult.join(10, java.util.concurrent.TimeUnit.SECONDS);
100+
assertTrue(shutdownResult.isDone(), "CompletableResultCode should be done after waiting");
112101
assertTrue(shutdownResult.isSuccess(), "Shutdown should complete successfully");
113-
114-
// Check threads after shutdown
115-
Set<Thread> threadsAfterShutdown = getOkHttpThreads();
116-
logger.info(
117-
"OkHttp threads after shutdown: "
118-
+ threadsAfterShutdown.size()
119-
+ " threads: "
120-
+ threadsAfterShutdown.stream().map(Thread::getName).collect(Collectors.joining(", ")));
121-
122-
// Find alive threads
123-
Set<Thread> aliveThreads =
124-
threadsAfterShutdown.stream().filter(Thread::isAlive).collect(Collectors.toSet());
125-
126-
// Separate dispatcher threads (HTTP call threads) from TaskRunner threads (internal OkHttp
127-
// threads)
128-
Set<Thread> dispatcherThreads =
129-
aliveThreads.stream()
130-
.filter(t -> t.getName().toLowerCase(Locale.ROOT).contains("dispatch"))
131-
.collect(Collectors.toSet());
132-
133-
Set<Thread> taskRunnerThreads =
134-
aliveThreads.stream()
135-
.filter(t -> t.getName().toLowerCase(Locale.ROOT).contains("taskrunner"))
136-
.collect(Collectors.toSet());
137-
138-
if (!aliveThreads.isEmpty()) {
139-
logger.info("Found " + aliveThreads.size() + " alive OkHttp threads after shutdown:");
140-
aliveThreads.forEach(
141-
t -> {
142-
logger.info(
143-
" - "
144-
+ t.getName()
145-
+ " (daemon: "
146-
+ t.isDaemon()
147-
+ ", state: "
148-
+ t.getState()
149-
+ ")");
150-
});
151-
}
152-
153-
// The main requirement: dispatcher threads (HTTP call threads) should be terminated
154-
assertTrue(
155-
dispatcherThreads.isEmpty(),
156-
"Dispatcher threads (HTTP call threads) should be terminated after shutdown. Found "
157-
+ dispatcherThreads.size()
158-
+ " alive dispatcher threads: "
159-
+ dispatcherThreads.stream().map(Thread::getName).collect(Collectors.joining(", ")));
160-
161-
// TaskRunner threads are OkHttp's internal idle daemon threads. They have a 60-second
162-
// keep-alive and will terminate on their own. They're harmless since:
163-
// 1. They're daemon threads (won't prevent JVM exit)
164-
// 2. They're idle (in TIMED_WAITING state)
165-
// 3. No new work can be dispatched to them after shutdown
166-
// We log them for visibility but don't fail the test.
167-
if (!taskRunnerThreads.isEmpty()) {
168-
logger.info(
169-
"Note: "
170-
+ taskRunnerThreads.size()
171-
+ " TaskRunner daemon threads are still alive. "
172-
+ "These are OkHttp internal threads that will terminate after their keep-alive timeout (60s). "
173-
+ "They won't prevent JVM exit.");
174-
}
175-
}
176-
177-
/** Get all threads that appear to be OkHttp-related. */
178-
private static Set<Thread> getOkHttpThreads() {
179-
ThreadGroup rootGroup = Thread.currentThread().getThreadGroup();
180-
while (rootGroup.getParent() != null) {
181-
rootGroup = rootGroup.getParent();
182-
}
183-
184-
Thread[] threads = new Thread[rootGroup.activeCount() * 2];
185-
int count = rootGroup.enumerate(threads, true);
186-
187-
Set<Thread> okHttpThreads = new java.util.HashSet<>();
188-
for (int i = 0; i < count; i++) {
189-
Thread thread = threads[i];
190-
if (thread != null && thread.getName() != null) {
191-
String name = thread.getName().toLowerCase(Locale.ROOT);
192-
if (name.contains("okhttp") || name.contains("ok-http")) {
193-
okHttpThreads.add(thread);
194-
}
195-
}
196-
}
197-
return okHttpThreads;
198102
}
199103

200104
/** Simple test marshaler for testing purposes. */

0 commit comments

Comments
 (0)