From 0359a4c73f5035482fbbde75c992130683c89563 Mon Sep 17 00:00:00 2001 From: Petri Klemela Date: Wed, 2 Sep 2026 12:48:54 +0000 Subject: [PATCH 1/3] Shut down leaked executors and Hibernate SessionFactory on close ZipSessionServlet and FileStorageDiscovery each created a cached thread pool that was never shut down. OidcResourceTest built its own HibernateUtil but never closed its SessionFactory. --- src/main/java/fi/csc/chipster/filebroker/FileBroker.java | 1 + .../chipster/filestorage/client/FileStorageDiscovery.java | 4 ++++ .../fi/csc/chipster/sessionworker/ZipSessionServlet.java | 5 +++++ src/test/java/fi/csc/chipster/auth/OidcResourceTest.java | 1 + 4 files changed, 11 insertions(+) diff --git a/src/main/java/fi/csc/chipster/filebroker/FileBroker.java b/src/main/java/fi/csc/chipster/filebroker/FileBroker.java index e6572521..8470f695 100644 --- a/src/main/java/fi/csc/chipster/filebroker/FileBroker.java +++ b/src/main/java/fi/csc/chipster/filebroker/FileBroker.java @@ -143,6 +143,7 @@ public void close() { try { httpServer.stop(); authService.close(); + storageDiscovery.close(); } catch (Exception e) { logger.warn("failed to stop the file-broker", e); } diff --git a/src/main/java/fi/csc/chipster/filestorage/client/FileStorageDiscovery.java b/src/main/java/fi/csc/chipster/filestorage/client/FileStorageDiscovery.java index 82462673..73f3feae 100644 --- a/src/main/java/fi/csc/chipster/filestorage/client/FileStorageDiscovery.java +++ b/src/main/java/fi/csc/chipster/filestorage/client/FileStorageDiscovery.java @@ -280,4 +280,8 @@ public Map getStorages() { return storages; } } + + public void close() { + updateExecutor.shutdown(); + } } diff --git a/src/main/java/fi/csc/chipster/sessionworker/ZipSessionServlet.java b/src/main/java/fi/csc/chipster/sessionworker/ZipSessionServlet.java index f17a5a62..5b52dfee 100644 --- a/src/main/java/fi/csc/chipster/sessionworker/ZipSessionServlet.java +++ b/src/main/java/fi/csc/chipster/sessionworker/ZipSessionServlet.java @@ -112,6 +112,11 @@ public ZipSessionServlet(ServiceLocatorClient serviceLocator) { this.executor = Executors.newCachedThreadPool(); } + @Override + public void destroy() { + executor.shutdown(); + } + private void packageSession(HttpServletResponse response, StaticCredentials credentials, UUID sessionId) throws IOException { diff --git a/src/test/java/fi/csc/chipster/auth/OidcResourceTest.java b/src/test/java/fi/csc/chipster/auth/OidcResourceTest.java index 9bf491d0..0a0e2fc5 100644 --- a/src/test/java/fi/csc/chipster/auth/OidcResourceTest.java +++ b/src/test/java/fi/csc/chipster/auth/OidcResourceTest.java @@ -117,6 +117,7 @@ public static void setUp() throws Exception { @AfterAll public static void tearDown() throws Exception { + hibernate.getSessionFactory().close(); launcher.stop(); } From 1cdf60815ad65533f8a8ed083492aef4d76586f0 Mon Sep 17 00:00:00 2001 From: Taavi Hupponen Date: Mon, 7 Sep 2026 19:32:15 +0000 Subject: [PATCH 2/3] Shut down the file-broker components independently on close storageDiscovery.close() shared a try block with httpServer.stop() and authService.close(), so a failure in either left the executor running, i.e. the leak survived exactly the case it was added for. Give each step its own try/catch instead. The executor shutdown still runs after httpServer.stop(), because requests reach FileStorageDiscovery.updateInBackgroundIfNecessary() and would get a RejectedExecutionException. The null checks keep close() usable from main() when startServer() failed before these fields were assigned, which the single broad catch used to handle implicitly. --- .../csc/chipster/filebroker/FileBroker.java | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/main/java/fi/csc/chipster/filebroker/FileBroker.java b/src/main/java/fi/csc/chipster/filebroker/FileBroker.java index 8470f695..167a075f 100644 --- a/src/main/java/fi/csc/chipster/filebroker/FileBroker.java +++ b/src/main/java/fi/csc/chipster/filebroker/FileBroker.java @@ -140,12 +140,26 @@ public static void main(String[] args) throws Exception { public void close() { RestUtils.shutdown("file-broker-admin", adminServer); + try { - httpServer.stop(); - authService.close(); - storageDiscovery.close(); + if (httpServer != null) { + httpServer.stop(); + } } catch (Exception e) { logger.warn("failed to stop the file-broker", e); } + + try { + if (authService != null) { + authService.close(); + } + } catch (Exception e) { + logger.warn("failed to stop the file-broker auth client", e); + } + + // after httpServer.stop(), because requests submit tasks to its executor + if (storageDiscovery != null) { + storageDiscovery.close(); + } } } From 65073cb371aa7e8426a40a4a249437b3a6294172 Mon Sep 17 00:00:00 2001 From: Taavi Hupponen Date: Tue, 8 Sep 2026 10:25:14 +0000 Subject: [PATCH 3/3] Survive a rejected zip task and always release the export resources destroy() shuts down the executor, so executor.submit() can throw RejectedExecutionException while an export is still running. Only RestException was caught, so it escaped packageSession: the latch was never counted down and the keep-alive thread kept writing spaces to the client, and the temporary zip dataset was left in the session. A rejected zip task is now an ordinary error instead, which deletes the dataset and sends the errors in the json like every other failure. The upload got its own errors.isEmpty() check, because it must not read a pipe that no thread is writing to. Release the keep-alive and the pipe in a finally, which covers unchecked exceptions from anywhere in the method. Closing the read end also unblocks the zip thread when the upload failed while it was still writing: nobody reads the pipe after that, but PipedOutputStream.write waits forever, because the reader thread stays alive in the Jetty pool. The keep-alive submit is intentionally left unguarded. It runs before anything is written, so a rejection there still fails the request cleanly, which is better than importing or exporting a session with no keep-alive when the router closes the connection in 30 seconds. --- .../sessionworker/ZipSessionServlet.java | 45 ++++++++++++++----- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/src/main/java/fi/csc/chipster/sessionworker/ZipSessionServlet.java b/src/main/java/fi/csc/chipster/sessionworker/ZipSessionServlet.java index 5b52dfee..43237774 100644 --- a/src/main/java/fi/csc/chipster/sessionworker/ZipSessionServlet.java +++ b/src/main/java/fi/csc/chipster/sessionworker/ZipSessionServlet.java @@ -17,6 +17,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import java.util.zip.ZipEntry; @@ -128,6 +129,9 @@ private void packageSession(HttpServletResponse response, StaticCredentials cred ArrayList entries = new ArrayList<>(); + CountDownLatch latch = new CountDownLatch(1); + PipedInputStream in = null; + try { Session session = sessionDb.getSession(sessionId); @@ -142,8 +146,6 @@ private void packageSession(HttpServletResponse response, StaticCredentials cred ArrayList errors = new ArrayList<>(); - CountDownLatch latch = new CountDownLatch(1); - OutputStream respoonseOutput = response.getOutputStream(); keepAliveWithSpaces(respoonseOutput, latch); @@ -186,20 +188,30 @@ private void packageSession(HttpServletResponse response, StaticCredentials cred } OutputStream output2 = new PipedOutputStream(); - PipedInputStream in = new PipedInputStream((PipedOutputStream) output2); + in = new PipedInputStream((PipedOutputStream) output2); if (errors.isEmpty()) { // start creating the zip stream in background thread (may complete before all // data is uploaded) - executor.submit(() -> { - try { - streamZip(entries, output2); - } catch (IOException e) { - logger.error("failed to package zip session", e); - errors.add("failed to package zip session: " + e.getMessage()); - } - }); + try { + executor.submit(() -> { + try { + streamZip(entries, output2); + } catch (IOException e) { + logger.error("failed to package zip session", e); + errors.add("failed to package zip session: " + e.getMessage()); + } + }); + } catch (RejectedExecutionException e) { + // destroy() has shut down the executor, i.e. this session-worker is stopping + logger.error("failed to start zip packaging, session-worker is stopping", e); + errors.add("failed to package zip session: session-worker is stopping"); + } + } + // skipped when the zip thread wasn't started, because then the upload would + // block forever waiting for the zip stream + if (errors.isEmpty()) { // upload in the zip stream in this thread, so that we send response to this // servlet request only after the upload has really completed try { @@ -232,12 +244,23 @@ private void packageSession(HttpServletResponse response, StaticCredentials cred logger.info("response: " + RestUtils.asJson(json, true)); + // stop the keep-alive before writing the json. This doesn't wait for a write + // that is already in progress, but prevents all the following ones. latch.countDown(); respoonseOutput.write(RestUtils.asJson(json).getBytes()); respoonseOutput.close(); } catch (RestException e) { throw ServletUtils.extractRestException(e); + } finally { + // stop the keep-alive thread also when something unexpected was thrown. + // Otherwise it would keep writing spaces to the client forever. + latch.countDown(); + + // If the upload failed or was skipped, the zip thread may still be writing to + // the pipe. Its writes would block forever, because the reader thread stays + // alive in the Jetty pool. Closing the read end makes them fail instead. + IOUtils.closeQuietly(in); } }