Skip to content

Shut down leaked executors and Hibernate SessionFactory on close - #37

Open
klemela wants to merge 3 commits into
masterfrom
fix/leaked-executors-and-session-factory
Open

Shut down leaked executors and Hibernate SessionFactory on close#37
klemela wants to merge 3 commits into
masterfrom
fix/leaked-executors-and-session-factory

Conversation

@klemela

@klemela klemela commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

ZipSessionServlet and FileStorageDiscovery each created a cached
thread pool that was never shut down. OidcResourceTest built its own
HibernateUtil but never closed its SessionFactory.

Test plan

No dedicated test: each of these is created once for the life of its
owning process (SessionWorker, FileBroker, the test JVM), so the leak
only matters if that owner is created/destroyed repeatedly, which
doesn't happen in normal use or in the test suite. Verified with the
full suite still passing.

ZipSessionServlet and FileStorageDiscovery each created a
cached thread pool that was never shut down. OidcResourceTest built
its own HibernateUtil but never closed its SessionFactory.
@klemela
klemela requested a review from hupponen September 2, 2026 12:58
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.
@hupponen

hupponen commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Pushed 1cdf6081 to address the shutdown ordering in FileBroker.close().

storageDiscovery.close() shared a try block with httpServer.stop() and authService.close(), so if either of those threw, the executor was never shut down — the leak survived exactly the failure case the line was added for. Each step now gets its own try/catch, so one failure no longer skips the rest of the shutdown.

The executor shutdown deliberately stays after httpServer.stop(): requests reach FileStorageDiscovery.updateInBackgroundIfNecessary(), which submits to that executor, so stopping it first would let in-flight requests hit a RejectedExecutionException. There's a comment on the line recording that constraint.

The null checks preserve behaviour the single broad catch was providing implicitly: main() calls close() when startServer() fails, and httpServer / authService / storageDiscovery can all still be null at that point. Without the guards an NPE would escape close() and skip the System.exit(1). This matches how RestUtils.shutdown() already tolerates a null server.

./gradlew compileJava passes.

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.
@hupponen

hupponen commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Pushed 65073cb3, which makes the export survive the executor shutdown that destroy() now performs.

executor.submit() throws RejectedExecutionException once destroy() has run, and only RestException was caught, so it escaped packageSession: latch.countDown() never ran and the keep-alive thread went on writing spaces to the client, while the temporary zip dataset stayed in the session.

A rejected zip task is now an ordinary entry in errors, so it takes the same path as every other failure — the incomplete dataset is deleted and the errors go out in the JSON. The upload needed its own errors.isEmpty() check: it must not read a pipe that no thread is writing to, or it would block indefinitely instead.

The keep-alive and the pipe are released in a finally, which also covers unchecked exceptions from anywhere in the method. Closing the pipe's read end has a second effect worth noting: when the upload fails while the zip thread is still writing, nothing reads the pipe again, but PipedOutputStream.write waits forever, because PipedInputStream only fails the writer on "Pipe closed" or "Read end dead" and the reader is a Jetty pool thread that stays alive. That stranded one pool thread plus an open file-broker download per failed export.

The keep-alive submit() is deliberately left unguarded. It runs before anything is written to the response, so a rejection there fails the request cleanly with a 500 — better than continuing an import or export with no keep-alive, which the router terminates after 30 s while the server carries on and commits the session anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants