Skip to content

Fix reference type and avoid closing jar directly#6741

Merged
headius merged 1 commit intojruby:jruby-9.2from
headius:backport_soft_jarcache
Jul 4, 2021
Merged

Fix reference type and avoid closing jar directly#6741
headius merged 1 commit intojruby:jruby-9.2from
headius:backport_soft_jarcache

Conversation

@headius
Copy link
Member

@headius headius commented Jul 4, 2021

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 #6218. This change is not sufficient to fix all causes of
those errors but may prevent errors from JarCache.

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.
@headius headius merged commit 6c13f4c into jruby:jruby-9.2 Jul 4, 2021
@headius headius deleted the backport_soft_jarcache branch July 4, 2021 17:21
@enebo enebo added this to the JRuby 9.2.20.0 milestone Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants