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.
When reading this code recently, I noticed we were using some custom classes like
AsyncFutureImpl
andFutureAdapter
which now have equivalents in the Java Platform. I thought the code was easier to read when using the standard classes rather than our custom ones.This is a tiny public API change for
hudson.remoting.JarCache
and a handful of package-private internal classes, but I searched injenkinsci
andcloudbees
and found no consumers outside of Remoting itself, so this change seems safe from an API compatibility point of view.I stepped through the code in the debugger before and after and found one minor change: when downloading a JAR for the first time, the chained future to read the file into memory from
ResourceImageInJar
runs immediately as part of thepromise.complete(url)
call inJarCacheSupport
on theAtmostOneThreadExecutor
thread rather than before where it was run lazily a little bit later when calling.get()
fromRemoteClassLoader#loadRemoteClass
as part of the request handling thread. This doesn't seem like it should make any difference to me, and I think the readability benefits of usingCompletableFuture
everywhere are worth the minor change in behavior. But if people feel this is too risky, I can always roll back the call toCompletableFuture#thenApply
inResourceImageInJar
and go back to using Kohsuke'sFutureAdapter
which would restore the old behavior. In any case, this only matters when the cache is cold; when the cache is warm, the behavior is identical to the old behavior (including the horrific existing behavior of reading the entire JAR file from disk for each.class
file load operation, a great stress test of the OS page cache!).All in all this change shuffles a few things around internally within the implementation but shouldn't result in any change in user-visible behavior, and in my opinion modernizes the codebase a little and improves readability, but if people feel the risk is too high, I could make this change somewhat more conservative as described above, or even close this PR entirely if people would rather not touch this messy code at all.
Testing done
Ran
mvn clean verify
, and ran a Pipeline job with the Git plugin and JGit with both warm and cold JAR caches, stepping through the relevant functionality in both cases in the debugger to verify correct operation before and after this change.