pjfanning commented on PR #1217: URL: https://github.com/apache/pekko-http/pull/1217#issuecomment-5479156549
Rebased onto main and pushed 300cacafa 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 #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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
