The GitHub Actions job "Validate and test" on pekko-http.git/main has succeeded. Run started by GitHub user pjfanning (triggered by pjfanning).
Head commit for run: c7f3b45818119c176f72a31fa6b5166d9e56d4b2 / PJ Fanning <[email protected]> Avoid reopening jars per resource request and deleteOnExit per upload (#1217) * serve jar resources without reopening the jar per request 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 * don't register every uploaded temp file with deleteOnExit 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 * make the jar file cache for resource serving configurable 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 * address review: shutdown ordering, jar connection ownership, config claims 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 #1242 - keeps the threat model's "registers no JVM shutdown hook" claim true Report URL: https://github.com/apache/pekko-http/actions/runs/33484473164 With regards, GitHub Actions via GitBox --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
