Fix cleanResponses must handle cleaning up cancelled future (#1218)

* Fix `cleanResponses` must handle cleaning up cancelled future If a future would be cancelled outside of `cleanResponses`, calling `future.get()` would throw a `CancellationException`. This exception was not caught, breaking future cleanup. Signed-off-by: Maurice van Veen <[email protected]> * Catch Throwable Signed-off-by: Maurice van Veen <[email protected]> --------- Signed-off-by: Maurice van Veen <[email protected]>
nats-io · Aug 23, 2024 · 3198cba · 3198cba
1 parent b2ceb1a
commit 3198cba
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 1 deletion.
diff --git a/src/main/java/io/nats/client/impl/NatsConnection.java b/src/main/java/io/nats/client/impl/NatsConnection.java
@@ -1171,7 +1171,7 @@ else if (future.isDone()) {
                     wasInterrupted = true;
                     break;
                 }
-                catch (ExecutionException ignore) {}
+                catch (Throwable ignore) {}
             }
 
             if (remove) {

diff --git a/src/test/java/io/nats/client/impl/RequestTests.java b/src/test/java/io/nats/client/impl/RequestTests.java
@@ -739,4 +739,24 @@ public void testNatsImplAndEmptyStatsCoverage() {
         assertEquals(0, s.getDroppedCount());
         assertEquals(0, s.getReconnects());
     }
+
+    @Test
+    public void testCancelledFutureMustNotErrorOnCleanResponses() throws Exception {
+        try (NatsTestServer ts = new NatsTestServer(false)) {
+            Options options = Options.builder()
+                    .server(ts.getURI())
+                    .noNoResponders()
+                    .requestCleanupInterval(Duration.ofSeconds(10))
+                    .build();
+            NatsConnection nc = (NatsConnection) Nats.connect(options);
+
+            NatsRequestCompletableFuture future = (NatsRequestCompletableFuture) nc.request("request", null);
+            future.cancelClosing();
+
+            // Future is already cancelled, collecting it shouldn't result in an exception being thrown.
+            assertDoesNotThrow(() -> {
+                nc.cleanResponses(false);
+            });
+        }
+    }
 }