Caffeine: NullPointerException in LocalCacheFactory with JDK 8 when caffeine is on the bootclasspath

[cushon]

Originally report as: https://github.com/google/error-prone/issues/1986

When running on JDK 8 with caffeine on the bootclasspath (e.g. using -Xbootclasspath/a:), caffeine crashes with an NPE in LocalCacheFactory.

Specifically LocalCacheFactory.class.getClassLoader() returns null on this line:

https://github.com/ben-manes/caffeine/blob/f77b9e213f101ec26dffdeb962e74c81ede54650/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheSelectorCode.java#L102

The javadoc for Class.getClassLoader allows this:

Some implementations may use null to represent the bootstrap class loader

One way to handle this would be to fall back to using e.g. ClassLoader.getSystemClassLoader().

[ben-manes]

Can you provide a self-contained test clase / project that I could debug and apply your suggested fix against?

[cushon]

import com.github.benmanes.caffeine.cache.Caffeine;

class T {
  public static void main(String[] args) {
    Caffeine.newBuilder().maximumSize(1000).build();
  }
}

$ java -fullversion
java full version "1.8.0_202-b08"
$ wget https://repo1.maven.org/maven2/com/github/ben-manes/caffeine/caffeine/2.8.7/caffeine-2.8.7.jar
$ javac T.java -cp caffeine-2.8.7.jar 
$ java -Xbootclasspath/a:caffeine-2.8.7.jar T
Exception in thread "main" java.lang.NullPointerException
        at com.github.benmanes.caffeine.cache.LocalCacheFactory.newBoundedLocalCache(LocalCacheFactory.java:87)
        at com.github.benmanes.caffeine.cache.BoundedLocalCache$BoundedLocalManualCache.<init>(BoundedLocalCache.java:3353)
        at com.github.benmanes.caffeine.cache.BoundedLocalCache$BoundedLocalManualCache.<init>(BoundedLocalCache.java:3349)
        at com.github.benmanes.caffeine.cache.Caffeine.build(Caffeine.java:996)
        at T.main(T.java:5)

[ben-manes]

thanks! I'll test this and your suggestion tonight.

[cushon]

Thanks!

Thinking about my suggested workaround a little harder, instead of using ClassLoader.getSystemClassLoader(), it might be better to just use Class.forName(sb.toString()).

Class.forName(String) uses "the defining class loader of the current class", so it should handle the defining class loader of LocalCacheFactory being the platform class loader.

[ben-manes]

I verified that Class.forName passes your test.

I'm not sure why getClassLoader was used, as that was via a contributor. I recall a lot of messiness in early Java days due to different classloaders (system, context, thread) and generally tried to avoid too much of that as they interacted badly in application servers. Are you familiar enough to confirm that we should use forName rather than fallback across different classloaders to find an appropriate fit?

[ben-manes]

For example, in JCache we have this custom ClassLoader to be compatible with OSGi, JEE, etc but again based on contributors, as I haven't worked in those environments where it was wrangled with.

[cushon]

I think I've convinced myself that forName is the right choice here, even if there are other unusual class loading scenarios going on, e.g. caffeine is being loaded by a custom classloader.

The javadoc says Class.forName(String) is equivalent to Class.forName(className, true, currentLoader), where currentLoader "denotes the defining class loader of the current class". And Class.forName(String, boolean, ClassLoader) says if "parameter loader is null, the class is loaded through the bootstrap class loader".

So I think that is exactly the same as LocalCacheFactory.class.getClassLoader().loadClass(...) if LocalCacheFactory was loaded by anything other than the bootstrap class loader, and if LocalCacheFactory was loaded by the bootstrap class loader it's equivalent to ClassLoader.getSystemClassLoader().load(...).

[ben-manes]

okay, great. That is what I read it as too, but then have no confidence in my understanding of class loading to have picked it.

[ben-manes]

Release 2.8.8

[slandelle]

ClassLoader#loadClass and Class#forName don't exactly do the same thing. The latter will trigger the static initializers, the former won't. I can't tell if it matters for you, depending on the classes you're trying to load.

[ben-manes]

It delays until first use if I understood correctly. For this usage that shouldn’t matter. The goal is simply to avoid memory overhead of unused configuration fields by code generating the variations and only class load what is needed. That led to using reflection, but otherwise it is very unmagical.

[slandelle]

All good then :)