Backport JarCache soft reference fix #6734
Merged
Conversation
This file contains 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
This cache appears to have thread-safety issues. It is currently used as a global (aka static) cache of all JarEntry from a given jar file, to reduce the costs of searching and loading resources. However it breaks when used across runtimes due to the JRubyClassLoader shutdown sequence removing and thereby closing jar files without consideration for whether they may be in use by another runtime on the same JVM. As far as I can tell, the jars being cached and explicitly removed in this way should not conflict across runtimes, since they are unpacked into unique temporary files, but the bug in JarCache is there nonetheless. There may be a way to ensure that a given index, and the JarFile reference it contains, are not and will not be in use by any other thread, allowing the file to be closed. But doing so as part of the shutdown of a runtime defeats a key goal of caching globally: reducing the cost of accessing the same jar from other runtimes. This patch makes two changes to the handling of these jar indices: * The old code used a WeakHashMap from String to JarIndex. But WHM weakly-references keys, not values, meaning that it would very quickly be vacated. This may or may not have impacted the likelihood of attempting to use a closed jar file. * Instead of eagerly cleaning up the jar indices, they are simply dereferenced and allowed to finalize at a later time. This ensures any live references to those indices will not find them prematurely closed. This was found while investigating jar resource errors reported in issue jruby#6218. This change is not sufficient to fix all causes of those errors but may prevent errors from JarCache.
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.
This PR backports #6268 to 9.2 to fix the evacuation of the cache (due to weakly referenced-keys that immediately get dereferenced).
This backport will make it easier to incorporate other improvements from @DirkMahler for #6730.