Avoid reopening jars per resource request and deleteOnExit per upload - #1217
Open
pjfanning wants to merge 3 commits into
Open
Avoid reopening jars per resource request and deleteOnExit per upload#1217pjfanning wants to merge 3 commits into
pjfanning wants to merge 3 commits into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.applyopened ajava.util.zip.ZipFilefor 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, andgetFromResource/getFromResourceDirectoryserved out of a jar is the usual production layout for static resources. The result ofgetEntrywas also dereferenced without a null check, so a missing entry would throw an NPE rather than reject.Uploads register a
deleteOnExitper file.fileUploadAllcalledFile.deleteOnExit()on each temporary upload file. The JVM keeps every path passed todeleteOnExitin 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
JarURLConnectionand 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 plainURLConnectionhandling with the fallback branch via a small helper.pekko.http.routing.use-jar-file-cachesetting (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 newResourceFile.apply(url, useJarFileCache)overload takes the flag.deleteOnExitper 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 ownZipFilefor the metadata but still streamed content throughURL.openStream, which uses the JDK caches.The on-exit cleanup that
fileUploadAlldocuments 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 permissionsFiles.createTempDirectoryapplies. Note thataddShutdownHookthrowsIllegalStateExceptiononce shutdown is in progress, so an upload arriving during shutdown fails —deleteOnExitthrew in the same situation, so this is parity rather than a regression.Scala and Java DSL settings are in parity,
@since 2.0.0is on the new public methods, and MiMa filters are added for the two newRoutingSettingsmembers (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 attoStrict), that a second request to the same route still works, and that a jar resource is still served correctly withuse-jar-file-cache = offsbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileUploadDirectivesSpec"- pass; new test asserts the temp files share one directorysbt http-tests/test- pass (1485 tests;TimeoutDirectivesSpecflaked once in a full run and passes on its own)sbt +http/compile- pass on 2.13.18 and 3.3.8sbt +http/mimaReportBinaryIssues- passsbt http/scalafmt http-tests/Test/scalafmt- cleanReferences
None - avoids reopening jars per resource request and an unbounded
deleteOnExitregistration per upload