Close http response, temp file and improve logging

mapfish · Dec 17, 2024 · 489514e · 489514e
1 parent 81ea097
commit 489514e
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 39 deletions.
diff --git a/core/src/main/java/org/mapfish/print/http/ConfigFileResolvingRequest.java b/core/src/main/java/org/mapfish/print/http/ConfigFileResolvingRequest.java
@@ -141,81 +141,89 @@ private ClientHttpResponse doFileRequest() throws IOException {
   }
 
   private ClientHttpResponse doHttpRequestWithRetry(final HttpHeaders headers) throws IOException {
-    AtomicInteger counter = new AtomicInteger();
+    final AtomicInteger counter = new AtomicInteger();
     do {
       logFetchingURIResource(headers);
       try {
-        ClientHttpResponse response = attemptToFetchResponse(headers, counter);
+        ClientHttpResponse response = attemptToFetchResponse(headers);
         if (response != null) {
           return response;
+        } else {
+          if (canRetry(counter)) {
+            sleepWithExceptionHandling();
+            LOGGER.debug("Retry fetching {}", this.getURI());
+          } else {
+            PrintException printException = new PrintException("Failed fetching " + getURI());
+            LOGGER.debug("Throwing exception since it cannot retry.", printException);
+            throw printException;
+          }
         }
       } catch (final IOException e) {
         handleIOException(e, counter);
+      } catch (final RuntimeException e) {
+        // handle other exceptions
+        handleIOException(new IOException(e), counter);
       }
     } while (true);
   }
 
   private void logFetchingURIResource(final HttpHeaders headers) {
     // Display headers, one by line <name>: <value>
     LOGGER.debug(
-        "Fetching URI resource {}, headers:\n{}",
+        "Fetching {}, using headers:\n{}",
         this.getURI(),
         headers.entrySet().stream()
             .map(entry -> entry.getKey() + "=" + String.join(", ", entry.getValue()))
             .collect(Collectors.joining("\n")));
   }
 
-  private ClientHttpResponse attemptToFetchResponse(
-      final HttpHeaders headers, final AtomicInteger counter) throws IOException {
+  private ClientHttpResponse attemptToFetchResponse(final HttpHeaders headers) throws IOException {
     ClientHttpRequest requestUsed =
         this.request != null ? this.request : createRequestFromWrapped(headers);
     LOGGER.debug("Executing http request: {}", requestUsed.getURI());
     ClientHttpResponse response = executeCallbacksAndRequest(requestUsed);
-    if (response.getRawStatusCode() < 500) {
-      LOGGER.debug(
-          "Fetching success URI resource {}, status code {}",
-          getURI(),
-          response.getRawStatusCode());
-      return response;
-    }
-    LOGGER.debug(
-        "Fetching failed URI resource {}, error code {}", getURI(), response.getRawStatusCode());
-    if (canRetry(counter)) {
-      sleepWithExceptionHandling();
-      LOGGER.debug("Retry fetching URI resource {}", this.getURI());
-    } else {
-      throw new PrintException(
-          String.format(
-              "Fetching failed URI resource %s, error code %s",
-              getURI(), response.getRawStatusCode()));
+    final int minStatusCodeError = HttpStatus.INTERNAL_SERVER_ERROR.value();
+    int rawStatusCode = minStatusCodeError;
+    try {
+      rawStatusCode = response.getRawStatusCode();
+      if (rawStatusCode < minStatusCodeError) {
+        LOGGER.debug("Successfully fetched {}, with status code {}", getURI(), rawStatusCode);
+        return response;
+      }
+      LOGGER.debug("Failed fetching {}, with error code {}", getURI(), rawStatusCode);
+      return null;
+    } finally {
+      if (rawStatusCode >= minStatusCodeError) {
+        response.close();
+      }
     }
-    return null;
   }
 
   private void handleIOException(final IOException e, final AtomicInteger counter)
       throws IOException {
 
     if (canRetry(counter)) {
       sleepWithExceptionHandling();
-      LOGGER.debug("Retry fetching URI resource {}", this.getURI());
+      LOGGER.debug("Retry fetching {} following exception", this.getURI(), e);
     } else {
-      LOGGER.debug("Fetching failed URI resource {}", getURI());
+      LOGGER.debug("Failed fetching {}", getURI());
       throw e;
     }
   }
 
   private void sleepWithExceptionHandling() {
+    int sleepMs = configFileResolvingHttpRequestFactory.getHttpRequestFetchRetryIntervalMillis();
+    LOGGER.debug("Sleeping {} ms before retrying.", sleepMs);
     try {
-      TimeUnit.MILLISECONDS.sleep(
-          configFileResolvingHttpRequestFactory.getHttpRequestFetchRetryIntervalMillis());
+      TimeUnit.MILLISECONDS.sleep(sleepMs);
     } catch (InterruptedException e) {
       Thread.currentThread().interrupt();
       throw new PrintException("Interrupted while sleeping", e);
     }
   }
 
   private boolean canRetry(final AtomicInteger counter) {
-    return counter.incrementAndGet() < getHttpRequestMaxNumberFetchRetry();
+    return counter.incrementAndGet() <= getHttpRequestMaxNumberFetchRetry();
   }
 
   private int getHttpRequestMaxNumberFetchRetry() {

diff --git a/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java b/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java
@@ -87,18 +87,22 @@ private CachedClientHttpResponse(final ClientHttpResponse originalResponse) thro
       this.headers = originalResponse.getHeaders();
       this.status = originalResponse.getRawStatusCode();
       this.statusText = originalResponse.getStatusText();
-      this.cachedFile =
+      this.cachedFile = createCachedFile(originalResponse.getBody());
+    }
+
+    private File createCachedFile(final InputStream originalBody) throws IOException {
+      File tempFile =
           File.createTempFile("cacheduri", null, HttpRequestFetcher.this.temporaryDirectory);
-      LOGGER.debug("Caching URI resource to {}", this.cachedFile);
-      try (OutputStream os = Files.newOutputStream(this.cachedFile.toPath())) {
-        InputStream body = originalResponse.getBody();
+      LOGGER.debug("Caching URI resource to {}", tempFile);
+      try (OutputStream os = Files.newOutputStream(tempFile.toPath())) {
         LOGGER.debug(
-            "Get from input stream {}, for response {}, body available: {}",
-            body.getClass(),
-            originalResponse.getClass(),
-            body.available());
-        IOUtils.copy(body, os);
+            "Get from input stream {}, body available: {}",
+            originalBody.getClass(),
+            originalBody.available());
+        IOUtils.copy(originalBody, os);
+        originalBody.close();
       }
+      return tempFile;
     }
 
     @Override

diff --git a/core/src/main/java/org/mapfish/print/servlet/job/impl/ThreadPoolJobManager.java b/core/src/main/java/org/mapfish/print/servlet/job/impl/ThreadPoolJobManager.java
@@ -452,10 +452,13 @@ private boolean updateRegistry() {
       final SubmittedPrintJob printJob = submittedJobs.next();
       if (!printJob.getReportFuture().isDone()
           && (isTimeoutExceeded(printJob) || isAbandoned(printJob))) {
-        LOGGER.info("Canceling job after timeout {}", printJob.getEntry().getReferenceId());
+        LOGGER.info(
+            "About to attempt timeout based automatic cancellation of job {}",
+            printJob.getEntry().getReferenceId());
         if (!printJob.getReportFuture().cancel(true)) {
           LOGGER.info(
-              "Could not cancel job after timeout {}", printJob.getEntry().getReferenceId());
+              "Automatic cancellation after timeout failed for job {}",
+              printJob.getEntry().getReferenceId());
         }
         // remove all canceled tasks from the work queue (otherwise the queue comparator
         // might stumble on non-PrintJob entries)