Cache Class instances that come from the library itself

[slovdahl]

Class loading and lookups can be costly in some contexts. For example, Tomcat can in some cases throw ClassNotFoundExceptions internally while looking up a class name, even if it has already been loaded. By caching Class instances for the classes coming from the library itself, we can avoid these exceptions (except once per class).

Inspiration taken from the Services cache.

[bdemers]

Thanks @slovdahl !
It's been a few since I dug through the Classes logic, but IIRC, there were a few odd edge cases when calls to forName could load implementations from different classloaders.

So you could end up a condition like:

1. Caller creates a new classloader and configures the thread's classloader
2. Call Classes.forName() - this loads a class from the `Thread.currentThread().getContextClassLoader()
3. Caller resets threads classloader (or otherwise removes the classloader)
4. New caller, calls Classes.forName(), (cache hit) this attempts to use an implementation from the now destroyed classloader.

I think at one point I had messed around with a more complex cache key.
I could be miss remembering some of the detail here (as I could be thinking about the internal Services class here too.

Any thoughts on this?

[lhazlewood]

Because of Classloaders, and their very different runtime contexts (web apps, native apps, OSGi, etc) we can't safely cache based on name alone, which is the purpose of the current logic that cascades in order of likelihood priority:

1. Current thread context classloader, followed by:
2. The classloader responsible for loading the Classes class itself, followed by:
3. The system classloader

Since (almost all?) name-based instantiation code in JJWT internals does not specify a classloader (on purpose, to allow this dynamic classloader technique), the existing loading logic is the only way to perform safe lookup. The other way is to use the Classloader as part of the cache key, but then we'd have to refactor all the code in the project to specify classloader at lookup time, which isn't feasible at all.

Additionally, it's not easily possible to know which classloader should be specified at lookup time, since, if it works on the first classloader within Tomcat, but it fails internally in Tomcat again, and JJWT falls back to another classloader, the two resulting classes would be different, and probably fail instantiation since the one needed (that previously worked before) wouldn't be the one found by Tomcat in that different context.

The current 3-classloader-cascade technique is used by most modern framework libraries that perform identical behavior - Spring, JBoss, etc. There's just no easy way around it if using name-only lookup.

Also, it's not clear to me how much of a problem this is in practice. Wouldn't any ClassNotFoundExceptions happen really only perhaps at application startup, and since these are often mostly negligible, things 'stabilize' after Tomcat is finished loading the app and wouldn't happen again?

This also concerns me:

Tomcat can in some cases throw ClassNotFoundExceptions internally while looking up a class name, even if it has already been loaded.

Without knowing more about why this happens in Tomcat, shouldn't this issue be reported so that Tomcat can fix it and not impose a burden on downstream libraries? If a class has been successfully resolved previously, why would Tomcat have this (non deterministic?) behavior?

Very confusing.

All of this said, I would be extremely hesitant to change this logic since it is the backbone of api-to-impl loading that has been working for the life of the project, and this type of concern hasn't been reported before. I'm not saying it's a perfect solution, but I am saying that the current logic seems to be fine for 99.99% of JJWT use cases. And given its breadth of impact, changing it could cause huge problems for the extended JJWT user/application base, so I'd rather not.

Perhaps another angle - if this Tomcat behavior is deterministic, i.e. we understand the context of how these things occur - perhaps the order of classloaders attempted can be configurable to avoid the ClassNotFoundException entirely?

I'd much rather work towards a deterministic solution than changing the current logic which is supposed to be a "things aren't deterministic, so this is the best/safest fallback" approach and works well for that purpose.

Thoughts?

[slovdahl]

Thanks for the quick feedback @bdemers and @lhazlewood!

I agree that this suggestion might not always work in case class loaders come and go, thanks for pointing that out. Usually though, class loaders delegate to their parent if a class is not found, so as long as class loaders are hierarchical, the same Class instance should be returned (unless jjwt-impl.jar is actually available from multiple class loaders, which is a bit nasty overall). But regardless, a 99% solution like the current state of this PR is not good enough for a library like JJWT.

The current 3-classloader-cascade technique is used by most modern framework libraries that perform identical behavior - Spring, JBoss, etc. There's just no easy way around it if using name-only lookup.

Yep 👍 One thing that I believe is different though (but please correct me if I'm wrong): it's not that common to do explicit class loading on paths that are potentially hot. My initial reaction when I noticed that JJWT does explicit class loading when calling e.g. Jwts.builder() was that it's quite an uncommon pattern to always go through explicit class loading calls.

(That said, I understand why it's done the way it is. Separating the API and implementation is really nice.)

Tomcat can in some cases throw ClassNotFoundExceptions internally while looking up a class name, even if it has already been loaded.

I probably should have made this part more clear. The exception that Tomcat throws internally is also caught by Tomcat itself, so no fallback takes place in JJWT. So in that sense Tomcat's behaviour is deterministic.

When I step through the code on our side it ends up calling findClass(name) here, but gets a ClassNotFoundException from it that it silently ignores before unconditionally trying the parent class loader and succeeds. It's possible that this could be improved on the Tomcat side.

There's actually another performance-related problem too that I didn't notice before. Tomcat also takes a lock right away in the loadClass(String name, boolean resolve) method. So every single call to a Jwts static method inside a specific Tomcat webapp goes through the same shared lock. I suspect that might be quite hard to fix on the Tomcat side.

this type of concern hasn't been reported before

A bit surprising indeed, but maybe there's something in our way of using that makes it stand out. This popped up in profiles when stress testing our application, so that's why I ended up here.

Wild idea, and I realize it would be a huge rewrite, but could the ServiceLoader mechanism used in io.jsonwebtoken.impl.lang.Services also be used for this purpose? 🤔 There we already have a cache in place.

[lhazlewood]

Thanks for the quick feedback @bdemers and @lhazlewood!

And thanks for the thoughtful discussion! I appreciate the work you've put into the discovery around this issue - it's a level of depth that we can't easily see, thank you 🙏 .

(unless jjwt-impl.jar is actually available from multiple class loaders, which is a bit nasty overall). But regardless, a 99% solution like the current state of this PR is not good enough for a library like JJWT.

You'd be surprised what we've seen with OSGi and things like dynamic webapp loading/unloading 😅 .

Yep 👍 One thing that I believe is different though (but please correct me if I'm wrong): it's not that common to do explicit class loading on paths that are potentially hot.

I don't know how common it is, but we have received issues related to dynamic classloader environments, so we're trying to ensure things 'just work' in all environments.

My initial reaction when I noticed that JJWT does explicit class loading when calling e.g. Jwts.builder() was that it's quite an uncommon pattern to always go through explicit class loading calls.

(That said, I understand why it's done the way it is. Separating the API and implementation is really nice.)

Yep! That's indeed the reason for the design.

When I step through the code on our side [Tomcat] ends up calling findClass(name) here, but gets a ClassNotFoundException from it that it silently ignores before unconditionally trying the parent class loader and succeeds. It's possible that this could be improved on the Tomcat side.

There's actually another performance-related problem too that I didn't notiice before. Tomcat also takes a lock right away in the loadClass(String name, boolean resolve) method. So every single call to a Jwts static method inside a specific Tomcat webapp goes through the same shared lock. I suspect that might be quite hard to fix on the Tomcat side.

This I think is the core of the issue, and regardless of how we might address things in JJWT (more on that below), it seems like it should be addressed with the Tomcat team. As most of us are aware, using exceptions for control flow is frowned upon, yet it is something being done in this particular part of Tomcat source code.

Further, the lock they acquire preemptively is the WebappClassLoader object instance itself, instead of using something more performant, perhaps like a ReentrantReadWriteLock.

It seems like their implementation should be improved to alleviate this problem, especially since it's all hidden from JJWT, or any other webapp or library that may use reflection after application start-up. For example, how many Spring apps experience this same issue since they can also use heavy reflection-based new instance creation (i.e. anything marked as Spring 'prototype' scope). JJWT would be just a tiny blip on the radar compared to that.

this type of concern hasn't been reported before

A bit surprising indeed, but maybe there's something in our way of using that makes it stand out. This popped up in profiles when stress testing our application, so that's why I ended up here.

I very much appreciate your stress testing efforts! That's always tedious (but valuable) work - thanks for reporting things 😄.

Wild idea, and I realize it would be a huge rewrite, but could the ServiceLoader mechanism used in io.jsonwebtoken.impl.lang.Services also be used for this purpose? 🤔 There we already have a cache in place.

I thought about this quite a bit this morning to see if I could find alternatives.

The Services/ServiceLoader approach could be used, but unfortunately it would be rather invasive, and cause a number of issues:

1. It's not just dynamic JwtBuilder instantiation we need to be concerned with. The same newInstance technique is used to create the following instances the same way:

   • JwtParserBuilder
   • JwtHeaderBuilder
   • ClaimsBuilder
   • JwkBuilder
   • JwkParserBuilder
   • JwkSetBuilder
   • JwkSetParserBuilder

   They would need to be addressed as well. (Thankfully, they're all created within the Jwts and Jwks utility classes, so they're easy enough to find).

2. When using java.util.ServiceLoader, a different interface needs to be defined for each one, e.g. JwtBuilderFactory, JwtParserBuilderFactory, JwtHeaderBuilderFactory, ClaimsBuilderFactory, etc, even though only a single interface would work, e.g. a Supplier.

3. All of those new *Factory interfaces must be in the public API which would 'pollute' things since there's no real reason for JJWT users to implement those interfaces (unlike JSON Serializer and Deserializer interfaces, which can happen enough with different applications).

But thinking through this more, and doing a 'find usages' for the Classes.newInstance method, there are two patterns within the JJWT API module:

1. The dynamic *Builder instantiation as described above, and what you're seeing in your tests, and:

2. Creating a static application singleton within the jjwt-api module's classes (like Jwts and Jwks). This is done to cache each algorithm registry for all the JWT Signature and Encryption algorithms, for example:

   

   

After seeing this, I wondered if we can do the following:

1. Still create all the above listed JwtBuilderFactory, JwtParserBuilderFactory concepts, but limit them to only be within the jjwt-impl module.

2. Make each one of those factory implementations implement the api's existing io.jsonwebtoken.lang.Supplier interface recently introduced in JJWT 0.12.0. This way no new API concepts need to be introduced.

3. Each factory's respective implementation of the get() method would use standard Java instantiation (no reflection) and return new DefaultJwtBuilder(), new DefaultJwtParserBuilder(), etc.

4. The Jwts and Jwks utility classes would still uses Classes.newInstance just as is done for the algorithm registries shown above, but creating the respective *Factory instances, referencing them as Suppliers. For example:

   private static final String BUILDER_FACTORY_FQCN = "io.jsonwebtoken.impl.DefaultJwtBuilderFactory";
   private static final Supplier<JwtBuilder> BUILDER_FACTORY = Classes.newInstance(BUILDER_FACTORY_FQCN);

5. The respective (existing) static utility factory methods would just delegate to the new factory .get() method(s):

   public static JwtBuilder builder() {
       return BUILDER_FACTORY.get();
   }

I think this would work because the factory instances would be cached in the api classes, and even if classloaders are replaced in a dynamic context, new static singleton factory instances would be created, and used by the code/context using the jjwt-api module. In other words, if a new classloader is created when interacting with the API, those new API concepts will be (re-)created, and there shouldn't be any class conflicts with the new api class usages (in the same way that static singleton Registry instances are working today).

Would you want to refactor your PR to try this technique? Or if it feels a little daunting, I'd be happy to do it as long as you're able to test the new code/artifacts in your test suite since I couldn't re-create that easily.

Thoughts?

[lhazlewood]

P.S. I forgot to mention:

While JwtParserBuilder, JwtHeaderBuilder, ClaimsBuilder, etc are indeed all different interfaces and theoretically could be loaded by java.util.ServiceLoader, we found that java.util.ServiceLoader has significant problems in high-usage (frequent instantiation) scenarios:

#752

So we created the Services class as a wrapper/workaround for these file I/O issues at scale, utilizing instance caching with the class as the cache key (which avoids classloader issues since classes are unique per classloader).

As such, using the internal JJWT Services class is only relevant for implementations that can be cached as (thread-safe) application singletons, and not for dynamic/stateful instances (like builders).

[slovdahl]

Thanks for idea! I'll need some time to digest your suggestion while juggling other priorities, but at the face of it it sounds like a great idea. I'll give it a try and ask for help if needed.