Skip to content

Avoid reopening jars per resource request and deleteOnExit per upload - #1217

Open
pjfanning wants to merge 4 commits into
apache:mainfrom
pjfanning:bugs
Open

Avoid reopening jars per resource request and deleteOnExit per upload#1217
pjfanning wants to merge 4 commits into
apache:mainfrom
pjfanning:bugs

Conversation

@pjfanning

@pjfanning pjfanning commented Aug 22, 2026

Copy link
Copy Markdown
Member

Two independent resource-handling fixes in the file and resource directives, one commit each (plus a follow-up commit that makes the first one configurable), so they can be split if you prefer separate PRs.

Motivation

Jar resources are reopened per request. ResourceFile.apply opened a java.util.zip.ZipFile for every request to a resource inside a jar, only to read the entry's size and time. That re-parses the whole central directory of the jar on each request, and getFromResource/getFromResourceDirectory served out of a jar is the usual production layout for static resources. The result of getEntry was also dereferenced without a null check, so a missing entry would throw an NPE rather than reject.

Uploads register a deleteOnExit per file. fileUploadAll called File.deleteOnExit() on each temporary upload file. The JVM keeps every path passed to deleteOnExit in a global set for the lifetime of the process, and the entry is not removed when the file is deleted after the stream is consumed, so a long-running server accepting uploads grows its heap by one entry per upload.

Modification

  • Read jar entry metadata from the JarURLConnection and leave its cache enabled, so the JDK reuses the same open jar the class loader already holds; guard against a null or vanished entry; share the plain URLConnection handling with the fallback branch via a small helper.
  • Add a pekko.http.routing.use-jar-file-cache setting (on by default) for that behaviour. With it off, the connection reading the metadata owns its jar file and closes it again, and the entity stream is opened through a connection with caches disabled too. ResourceFile.apply(url) keeps its previous meaning and uses the cache; the new ResourceFile.apply(url, useJarFileCache) overload takes the flag.
  • Place upload temp files in a directory of their own and register a single shutdown hook that removes it recursively, instead of one deleteOnExit per file.

Result

No jar is opened or parsed per request for jar-hosted resources, and a missing entry rejects instead of throwing. Deployments that need to replace jar files while the server runs (an open jar file cannot be replaced on Windows) can set use-jar-file-cache = off — which the previous implementation could not offer at all, since it opened its own ZipFile for the metadata but still streamed content through URL.openStream, which uses the JDK caches.

The on-exit cleanup that fileUploadAll documents is unchanged but now costs one shutdown hook per JVM rather than one permanent global entry per upload; the dedicated directory also gets the owner-only permissions Files.createTempDirectory applies. Note that addShutdownHook throws IllegalStateException once shutdown is in progress, so an upload arriving during shutdown fails — deleteOnExit threw in the same situation, so this is parity rather than a regression.

Scala and Java DSL settings are in parity, @since 2.0.0 is on the new public methods, and MiMa filters are added for the two new RoutingSettings members (needed on Scala 3).

Tests

  • sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec" - pass; new tests assert that the jar entry metadata matches the bytes actually served (a wrong length fails at toStrict), that a second request to the same route still works, and that a jar resource is still served correctly with use-jar-file-cache = off
  • sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileUploadDirectivesSpec" - pass; new test asserts the temp files share one directory
  • sbt http-tests/test - pass (1485 tests; TimeoutDirectivesSpec flaked once in a full run and passes on its own)
  • sbt +http/compile - pass on 2.13.18 and 3.3.8
  • sbt +http/mimaReportBinaryIssues - pass
  • sbt http/scalafmt http-tests/Test/scalafmt - clean

References

None - avoids reopening jars per resource request and an unbounded deleteOnExit registration per upload

pjfanning added a commit to potiuk/pekko-http that referenced this pull request Aug 31, 2026
Motivation:
Maintainer review found two claims that do not hold on main and one
internal inconsistency:
- §5 claimed Pekko HTTP writes no files, but fileUploadAll creates temp
  files and storeUploadedFile(s) writes entity bytes to an
  application-chosen destination (FileUploadDirectives.scala:178).
- §9 and §14 Q3 described safeDirectoryChildPath as containing
  traversal, but its canonical-path check compares strings, so a
  symlink resolving into a sibling directory that shares the served
  root as a string prefix escapes it (fix in flight in apache#1218).
- The status line said both "Q1-Q8 answered" and "all ten answered"
  while Q3 still ended in an open question, and the pinned commit was
  the PR's own first commit rather than the main commit reviewed.

Modification:
Restate the §5 file-system claim with the upload-directive carve-out
and correct Q9 accordingly. Answer Q3 as a correction: the escape is
VALID under §5b.4 and fixed by apache#1218; update §9 and the §15 back-map to
match. Note in §12 that apache#1217 would invalidate the shutdown-hook claim
on merge. Fix the status line, the tag tally, and the commit pin
(444d939 -> 85d7243, the main commit the branch is based on).

Result:
Every §5 negative claim matches the source at the pinned commit, Q3 is
answered consistently with the "all ten answered" status, and the two
in-flight PRs that touch the model's claims (apache#1217, apache#1218) are
cross-linked.

Tests:
Not run - docs only

References:
Refs apache#1218, Refs apache#1217
Motivation:
`ResourceFile` opened a `java.util.zip.ZipFile` for every request to a
resource that lives in a jar, only to read the entry's size and time. That
parses the whole central directory of the jar again per request, and
`getFromResource`/`getFromResourceDirectory` served from a jar is the usual
production layout for static resources. The result of `getEntry` was also
dereferenced without a null check.

Modification:
Read the metadata from the `JarURLConnection` instead and leave its cache
enabled, so the JDK reuses the same open jar file that the class loader
already holds. Guard against a null entry, and share the plain
`URLConnection` handling with the fallback branch.

Result:
No jar is opened or parsed per request for resources served from a jar, and
a missing entry rejects the request instead of throwing.

Tests:
- sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec" - pass, 1 new test asserting the entry metadata matches the bytes served
- sbt http-tests/test - pass
- sbt +http/compile - pass
- sbt http/mimaReportBinaryIssues - pass
- sbt http/scalafmt http-tests/Test/scalafmt - clean

References:
None - avoids reopening jars for every resource request
Motivation:
`fileUploadAll` called `File.deleteOnExit()` for each temporary upload file.
The JVM keeps every path passed to `deleteOnExit` in a global set for the
lifetime of the process, and the entry is not removed when the file itself
is deleted after the stream is consumed. A long-running server accepting
uploads therefore grows its heap by one entry per upload, forever.

Modification:
Put the temporary upload files in a directory of their own and register a
single shutdown hook that removes that directory recursively on exit.

Result:
The on-exit cleanup that the directive documents is unchanged, but it now
costs one shutdown hook per JVM instead of one permanent global entry per
uploaded file. The dedicated directory is created with the owner-only
permissions that `Files.createTempDirectory` applies.

Tests:
- sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileUploadDirectivesSpec" - pass, 1 new test asserting the temp files share one directory
- sbt http-tests/test - pass
- sbt +http/compile - pass
- sbt http/mimaReportBinaryIssues - pass
- sbt http/scalafmt http-tests/Test/scalafmt - clean

References:
None - removes an unbounded deleteOnExit registration per upload
Motivation:
Reading jar resource metadata through the JDK's jar file cache means the
jar file stays open for the lifetime of the process, which prevents the jar
from being replaced while the server runs (on Windows an open file cannot
be replaced). That should be a choice rather than something the directives
decide.

Modification:
Add a `pekko.http.routing.use-jar-file-cache` setting, on by default, and
pass it from `getFromResource` into `ResourceFile`. With the setting off,
the connection that reads the entry metadata owns its jar file and closes
it again, and the entity stream is opened through a connection with caches
disabled as well, so that nothing keeps the jar open between requests.
`ResourceFile.apply(url)` keeps its previous meaning and uses the cache.

Result:
The default is the cached behaviour, and deployments that need to replace
jar files at runtime can turn the cache off. Note that the previous
implementation could not offer that at all: it opened its own `ZipFile` for
the metadata but still streamed the content through `URL.openStream`, which
uses the JDK caches.

Tests:
- sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec" - pass, 1 new test serving a jar resource with the cache disabled
- sbt http-tests/test - pass (TimeoutDirectivesSpec flaked in the full run, passes on its own)
- sbt +http/mimaReportBinaryIssues - pass
- sbt http/scalafmt http-tests/Test/scalafmt - clean

References:
None - follow-up to the jar resource change on this branch
…laims

Motivation:
Review of the branch found problems in both halves. The raw
Runtime.addShutdownHook ran concurrently with CoordinatedShutdown's
hook, so during a graceful drain it could delete temp files that
in-flight uploads still use - deleteOnExit provably deleted only after
application hooks finished. The memoized upload directory was never
recreated if a temp-file reaper removed it while empty, failing all
later uploads until restart. In the jar path, the cache-off close was
not in a finally (leaking a JarFile per failing request), an explicit
setUseCaches(true) silently defeated an application-wide
URLConnection.setDefaultUseCaches(false), the public one-arg
ResourceFile.apply changed semantics by pinning jars in the JDK cache,
getContentLength truncated and turned unknown lengths into silent
empty 200s, an exception from the close in fromUrlConnection's finally
became a 500 where callers expect a rejection, and the cache decision
was spread over three hand-synchronized places. The use-jar-file-cache
documentation also wrongly claimed the JDK's jar cache is the class
loader's cache and implied disabling it makes classpath jars
replaceable.

Modification:
Make UploadTempFiles a per-actor-system extension whose directory is
removed by a CoordinatedShutdown task in the actor-system-terminate
phase, after the drain; recreate the directory in create() if it is
gone. In ResourceFile, restore the one-arg apply to its historical
no-handle-kept semantics (useJarFileCache = false), decide jar
ownership from the connection's effective getUseCaches, close the
owned jar in a guarded finally, only call setUseCaches when disabling,
centralize that rule in one openConnection helper shared with
openStream, dispatch on the connection type instead of the protocol
string, use getContentLengthLong and reject unknown lengths, and map
FileNotFoundException to None with a guarded stream close. Reword the
reference.conf entry to scope the replaceability promise to jars no
class loader holds open and document the two-parse cost of cache-off
mode. Rebased onto main.

Result:
Upload temp files are deleted only after the system has drained and
uploads keep working if the temp directory disappears; the jar path
neither leaks handles nor overrides application-wide cache opt-outs;
external ResourceFile(url) callers keep the pre-existing behavior; and
the configuration text makes no false claims about the JDK.

Tests:
- sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileUploadDirectivesSpec org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec" - pass (73 tests); new tests cover directory recreation after removal and directory deletion when a separate actor system terminates
- sbt http/mimaReportBinaryIssues - pass
- sbt "+http/compile" - pass on 2.13.18 and 3.3.8
- native scalafmt run on the changed Scala files - clean

References:
Refs apache#1242 - keeps the threat model's "registers no JVM shutdown hook" claim true
@pjfanning

Copy link
Copy Markdown
Member Author

Rebased onto main and pushed 300caca addressing the review findings:

  • Shutdown ordering: UploadTempFiles is now a per-actor-system extension whose directory is removed by a CoordinatedShutdown task in the actor-system-terminate phase — after in-flight requests drain, matching the ordering deleteOnExit used to guarantee. The raw Runtime.addShutdownHook ran concurrently with the drain and could delete files uploads were still using. Side benefit: Pekko HTTP registers no raw JVM hook of its own, so the §5 claim in the Add a draft threat model and wire it for discoverability #1242 threat model stays true.
  • Self-healing temp dir: create() recreates the directory if a temp-file reaper removed it while empty (it previously failed all uploads until restart). New test covers it, plus one asserting the directory is deleted when a separate actor system terminates.
  • Jar connection ownership: the cache-off close moved into a guarded finally (it leaked a JarFile on any non-FileNotFoundException failure); ownership is decided from the connection's effective getUseCaches, and setUseCaches is only called when disabling — so an application-wide setDefaultUseCaches(false) is no longer silently overridden (and the jar it forces open is now closed).
  • ResourceFile.apply(url): the public one-arg overload is restored to its historical no-handle-kept semantics (useJarFileCache = false) and documented; the caching behavior is reachable only through the two-arg overload driven by the setting.
  • Generic connection path: getContentLengthLong instead of the truncating getContentLength, negative (unknown) lengths reject instead of serving a silent empty 200, and FileNotFoundException maps to None with a guarded stream close instead of a 500.
  • Structure: dispatch is on the connection type rather than the protocol string, and the cache rule lives in one openConnection helper shared with openStream.
  • reference.conf: the "same cache that the class loader uses" claim was wrong (the JDK's JarFileFactory cache is disjoint from URLClassPath's handles), and cache-off cannot make a classpath jar replaceable — reworded to scope the promise to jars no class loader holds open, and to document the metadata+content two-parse cost of cache-off mode.

73 tests pass across FileUploadDirectivesSpec and FileAndResourceDirectivesSpec; http/mimaReportBinaryIssues and +http/compile (2.13.18, 3.3.8) pass.

pjfanning added a commit to potiuk/pekko-http that referenced this pull request Aug 31, 2026
… task

Motivation:
§12 flagged apache#1217's raw JVM shutdown hook as invalidating the §5
"registers no shutdown hook" claim on merge. That PR has since been
reworked to register its temp-file cleanup as a CoordinatedShutdown
task on the actor system instead, so no §5 claim is affected.

Modification:
Restate the §12 bullet as a recorded near-miss rather than a pending
invalidation.

Result:
§12 matches the current state of apache#1217 and the §5/§11a claims stand.

Tests:
Not run - docs only

References:
Refs apache#1217
pjfanning added a commit to potiuk/pekko-http that referenced this pull request Aug 31, 2026
Motivation:
Both in-flight PRs the model references changed shape after review.
apache#1218 gained a documented platform caveat - File.getCanonicalPath does
not resolve NTFS symbolic links or junctions on Windows, so the
link-escape class stays open there - and now rejects path segments that
no file-system path may contain instead of erroring. apache#1217 replaced the
per-file deleteOnExit with one temp directory per actor system removed
by a CoordinatedShutdown task. Q3's answer claimed symlink escapes are
rejected "whatever its target is named", which overclaims on Windows.

Modification:
Scope the Q3 and §9 symlink-rejection claims to platforms where
canonicalization resolves links, record the Windows residual and the
toRealPath follow-up, note the invalid-segment hardening, and update
the §5 upload-directive note to describe apache#1217's per-system directory
and CoordinatedShutdown cleanup.

Result:
The model's containment and file-writing claims match what apache#1217 and
apache#1218 actually implement, on every platform they address.

Tests:
Not run - docs only

References:
Refs apache#1217, Refs apache#1218
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.

1 participant