fix(timeout): remove HTTP timeout handler (#737) (#738)

* fix(timeout): remove HTTP timeout handler

Remove TimeoutHandler - this was initially used as a hack around preventing clients from seeing indefinitely-long request "hangs" when requesting URL paths that didn't exist, such as web-client assets in a minimal build. This is now properly handled by the WebClientAssetsHandler responding with a 404 in a minimal build, or else a 200 with the index.html in a non-minimal build. Other actual assets, like .js bundles, fonts, images, continue to be handled by the StaticAssetsHandler. This leaves no real case where the TimeoutHandler was useful. However, there were cases where the TimeoutHandler could be triggered in undesirable ways - for example, long-running report generation, archive uploads, jfr-datasource uploads, and recording downloads.

* test(timeout): correct broken unit tests

(cherry picked from commit 3706fe2)

Co-authored-by: Andrew Azores <aazores@redhat.com>

src/main/java/io/cryostat/net/reports/ActiveRecordingReportCache.java

@@ -38,7 +38,6 @@
 package io.cryostat.net.reports;
 
 import java.nio.file.Path;
-import java.time.Duration;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
@@ -56,7 +55,6 @@
 import io.cryostat.core.sys.FileSystem;
 import io.cryostat.net.ConnectionDescriptor;
 import io.cryostat.net.TargetConnectionManager;
-import io.cryostat.net.web.http.generic.TimeoutHandler;
 
 import com.github.benmanes.caffeine.cache.Caffeine;
 import com.github.benmanes.caffeine.cache.LoadingCache;
@@ -126,13 +124,7 @@ protected String getReport(SubprocessReportGenerator.RecordingDescriptor recordi
             generationLock.lock();
             logger.trace("Active report cache miss for {}", recordingDescriptor.recordingName);
             try {
-                saveFile =
-                        subprocessReportGeneratorProvider
-                                .get()
-                                .exec(
-                                        recordingDescriptor,
-                                        Duration.ofMillis(TimeoutHandler.TIMEOUT_MS))
-                                .get();
+                saveFile = subprocessReportGeneratorProvider.get().exec(recordingDescriptor).get();
                 return fs.readString(saveFile);
             } catch (ExecutionException | CompletionException ee) {
                 logger.error(ee);

src/main/java/io/cryostat/net/reports/ArchivedRecordingReportCache.java

@@ -39,7 +39,6 @@
 
 import java.io.IOException;
 import java.nio.file.Path;
-import java.time.Duration;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.Future;
 import java.util.concurrent.locks.ReentrantLock;
@@ -49,7 +48,6 @@
 
 import io.cryostat.core.log.Logger;
 import io.cryostat.core.sys.FileSystem;
-import io.cryostat.net.web.http.generic.TimeoutHandler;
 import io.cryostat.recordings.RecordingArchiveHelper;
 
 class ArchivedRecordingReportCache {
@@ -92,13 +90,7 @@ Future<Path> get(String recordingName) {
 
             Path archivedRecording = recordingArchiveHelper.getRecordingPath(recordingName).get();
             Path saveFile =
-                    subprocessReportGeneratorProvider
-                            .get()
-                            .exec(
-                                    archivedRecording,
-                                    dest,
-                                    Duration.ofMillis(TimeoutHandler.TIMEOUT_MS))
-                            .get();
+                    subprocessReportGeneratorProvider.get().exec(archivedRecording, dest).get();
             f.complete(saveFile);
         } catch (Exception e) {
             logger.error(e);

src/main/java/io/cryostat/net/reports/SubprocessReportGenerator.java

@@ -44,7 +44,6 @@
 import java.nio.file.Paths;
 import java.nio.file.StandardCopyOption;
 import java.nio.file.StandardOpenOption;
-import java.time.Duration;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
@@ -109,7 +108,7 @@ public class SubprocessReportGenerator {
         this.logger = logger;
     }
 
-    CompletableFuture<Path> exec(Path recording, Path saveFile, Duration timeout)
+    CompletableFuture<Path> exec(Path recording, Path saveFile)
             throws NoSuchMethodException, SecurityException, IllegalAccessException,
                     IllegalArgumentException, InvocationTargetException, IOException,
                     InterruptedException, ReportGenerationException {
@@ -142,7 +141,7 @@ CompletableFuture<Path> exec(Path recording, Path saveFile, Duration timeout)
         return CompletableFuture.supplyAsync(
                 () -> {
                     try {
-                        proc.waitFor(timeout.toMillis(), TimeUnit.MILLISECONDS);
+                        proc.waitFor(5, TimeUnit.MINUTES);
                         ExitStatus status = ExitStatus.byExitCode(proc.exitValue());
                         switch (status) {
                             case OK:
@@ -168,13 +167,13 @@ CompletableFuture<Path> exec(Path recording, Path saveFile, Duration timeout)
                 });
     }
 
-    Future<Path> exec(RecordingDescriptor recordingDescriptor, Duration timeout) throws Exception {
+    Future<Path> exec(RecordingDescriptor recordingDescriptor) throws Exception {
         Path recording =
                 getRecordingFromLiveTarget(
                         recordingDescriptor.recordingName,
                         recordingDescriptor.connectionDescriptor);
         Path saveFile = tempFileProvider.get();
-        CompletableFuture<Path> cf = exec(recording, saveFile, timeout);
+        CompletableFuture<Path> cf = exec(recording, saveFile);
         return cf.whenComplete(
                 (p, t) -> {
                     try {

src/main/java/io/cryostat/net/web/http/api/v1/TargetReportGetHandler.java

@@ -41,7 +41,6 @@
 import java.util.Set;
 import java.util.concurrent.CompletionException;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.TimeUnit;
 
 import javax.inject.Inject;
 
@@ -53,7 +52,6 @@
 import io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler;
 import io.cryostat.net.web.http.HttpMimeType;
 import io.cryostat.net.web.http.api.ApiVersion;
-import io.cryostat.net.web.http.generic.TimeoutHandler;
 import io.cryostat.recordings.RecordingNotFoundException;
 
 import io.vertx.core.http.HttpHeaders;
@@ -117,7 +115,7 @@ public void handleAuthenticated(RoutingContext ctx) throws Exception {
                     .end(
                             reportService
                                     .get(getConnectionDescriptorFromContext(ctx), recordingName)
-                                    .get(TimeoutHandler.TIMEOUT_MS, TimeUnit.MILLISECONDS));
+                                    .get());
         } catch (CompletionException | ExecutionException ee) {
 
             Exception rootCause = (Exception) ExceptionUtils.getRootCause(ee);

src/main/java/io/cryostat/net/web/http/generic/HttpGenericModule.java

@@ -46,10 +46,6 @@
 @Module
 public abstract class HttpGenericModule {
 
-    @Binds
-    @IntoSet
-    abstract RequestHandler bindTimeoutHandler(TimeoutHandler handler);
-
     @Binds
     @IntoSet
     abstract RequestHandler bindCorsEnablingHandler(CorsEnablingHandler handler);

src/test/java/io/cryostat/net/reports/ActiveRecordingReportCacheTest.java

@@ -38,7 +38,6 @@
 package io.cryostat.net.reports;
 
 import java.nio.file.Path;
-import java.time.Duration;
 import java.util.Set;
 import java.util.concurrent.CompletionException;
 import java.util.concurrent.ExecutionException;
@@ -114,8 +113,7 @@ void shouldReturnTrueWhenDeletingReport() throws Exception {
         Mockito.when(pathFuture.get()).thenReturn(destinationFile);
         Mockito.when(
                         subprocessReportGenerator.exec(
-                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class),
-                                Mockito.any(Duration.class)))
+                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class)))
                 .thenReturn(pathFuture);
         Mockito.when(fs.readString(destinationFile)).thenReturn(REPORT_DOC);
 
@@ -132,8 +130,7 @@ void shouldReturnGeneratedReportResult() throws Exception {
         Mockito.when(pathFuture.get()).thenReturn(destinationFile);
         Mockito.when(
                         subprocessReportGenerator.exec(
-                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class),
-                                Mockito.any(Duration.class)))
+                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class)))
                 .thenReturn(pathFuture);
         Mockito.when(fs.readString(destinationFile)).thenReturn(REPORT_DOC);
 
@@ -147,9 +144,7 @@ void shouldReturnGeneratedReportResult() throws Exception {
         inOrder.verify(lock).lock();
 
         inOrder.verify(subprocessReportGenerator)
-                .exec(
-                        Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class),
-                        Mockito.any(Duration.class));
+                .exec(Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class));
 
         inOrder.verify(fs).readString(destinationFile);
 
@@ -161,8 +156,7 @@ void shouldReturnCachedReportResultOnSecondRequest() throws Exception {
         Mockito.when(pathFuture.get()).thenReturn(destinationFile);
         Mockito.when(
                         subprocessReportGenerator.exec(
-                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class),
-                                Mockito.any(Duration.class)))
+                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class)))
                 .thenReturn(pathFuture);
         Mockito.when(fs.readString(destinationFile)).thenReturn(REPORT_DOC);
 
@@ -179,9 +173,7 @@ void shouldReturnCachedReportResultOnSecondRequest() throws Exception {
         inOrder.verify(lock, Mockito.times(1)).lock();
 
         inOrder.verify(subprocessReportGenerator, Mockito.times(1))
-                .exec(
-                        Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class),
-                        Mockito.any(Duration.class));
+                .exec(Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class));
 
         inOrder.verify(lock, Mockito.times(1)).unlock();
     }
@@ -191,8 +183,7 @@ void shouldThrowExceptionIfRecordingNotFound() throws Exception {
         ConnectionDescriptor connectionDescriptor = new ConnectionDescriptor("foo");
         Mockito.when(
                         subprocessReportGenerator.exec(
-                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class),
-                                Mockito.any(Duration.class)))
+                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class)))
                 .thenThrow(new CompletionException(new RecordingNotFoundException("", "")));
         Assertions.assertThrows(
                 ExecutionException.class, () -> cache.get(connectionDescriptor, "bar").get());
@@ -203,8 +194,7 @@ void shouldThrowExceptionIfSubprocessExitsNonCleanly() throws Exception {
         ConnectionDescriptor connectionDescriptor = new ConnectionDescriptor("foo");
         Mockito.when(
                         subprocessReportGenerator.exec(
-                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class),
-                                Mockito.any(Duration.class)))
+                                Mockito.any(SubprocessReportGenerator.RecordingDescriptor.class)))
                 .thenThrow(
                         new CompletionException(
                                 new SubprocessReportGenerator.ReportGenerationException(

src/test/java/io/cryostat/net/reports/ArchivedRecordingReportCacheTest.java

@@ -38,7 +38,6 @@
 package io.cryostat.net.reports;
 
 import java.nio.file.Path;
-import java.time.Duration;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
 import java.util.concurrent.ExecutionException;
@@ -122,9 +121,7 @@ void getShouldGenerateAndCacheReport() throws Exception {
         Mockito.when(pathFuture.get()).thenReturn(destinationFile);
         Mockito.when(
                         subprocessReportGenerator.exec(
-                                Mockito.any(Path.class),
-                                Mockito.any(Path.class),
-                                Mockito.any(Duration.class)))
+                                Mockito.any(Path.class), Mockito.any(Path.class)))
                 .thenReturn(pathFuture);
 
         Future<Path> res = cache.get(recordingName);
@@ -167,9 +164,7 @@ void shouldThrowErrorIfReportGenerationFails() throws Exception {
 
         Mockito.when(
                         subprocessReportGenerator.exec(
-                                Mockito.any(Path.class),
-                                Mockito.any(Path.class),
-                                Mockito.any(Duration.class)))
+                                Mockito.any(Path.class), Mockito.any(Path.class)))
                 .thenThrow(
                         new CompletionException(
                                 new SubprocessReportGenerator.ReportGenerationException(