Classloader leak: DEFAULT_ANNOTATION_INTROSPECTOR holds annotation reference (JBoss)

[ciis0]

Describe the bug
A clear and concise description of what the bug is.

Similar to #3093, in JBoss JacksonAnnotationIntrospector._annotationsInside, referenced statically via ObjectMapper.DEFAULT_ANNOTATION_INTROSPECTOR, might contain references to annotations from Deployment Class-Loaders, which leads to leaking those when the deployment is stopped/undeployed.

Version information
Which Jackson version(s) was this for?

2.13.4.2, but from the code it looks like it's still true today.

jackson-databind/src/main/java/com/fasterxml/jackson/databind/ObjectMapper.java

Line 375 in de62c67

protected final static AnnotationIntrospector DEFAULT_ANNOTATION_INTROSPECTOR = new JacksonAnnotationIntrospector();

jackson-databind/src/main/java/com/fasterxml/jackson/databind/introspect/JacksonAnnotationIntrospector.java

Line 82 in de62c67

protected transient LRUMap<Class<?>,Boolean> _annotationsInside = new LRUMap<Class<?>,Boolean>(48, 48);

jackson-databind/src/main/java/com/fasterxml/jackson/databind/introspect/JacksonAnnotationIntrospector.java

Line 162 in de62c67

_annotationsInside.putIfAbsent(type, b);

To Reproduce

https://github.com/ciis0/jackson-3771

Expected behavior
If reproduction itself needs further explanation, you may also add more details here.

Additional context
Add any other context about the problem here.

Reference to annotation class from deployment class-loader in JacksonAnnotationIntrospector instance:

Static reference to that annotation introspector instance from ObjectMapper:

[ciis0]

I have uploaded a reproducer here: https://github.com/ciis0/jackson-3771

[pjfanning]

I'd say this should be fixed by JBoss. That LRUMap can readily be garbage collected. @cowtowncoder might have his own opinion but to me, I would need to be convinced why this is not a JBoss issue.

[pjfanning]

In theory, Jackson could use a WeakHashMap instead of LRUMap but JBoss may still fail to clean up other Jackson refs.

[ciis0]

I'd say this should be fixed by JBoss. That LRUMap can readily be garbage collected.

Ah, this is hidden away in #3093: the LRUMap is (indirectly) referenced via a static field in ObjectMapper.DEFAULT_ANNOTATION_INTROSPECTOR (JacksonAnnotationIntrospector), that's why it is not cleaned up. :)

I have updated the description.

[ciis0]

i.e. the problem is not that the LURMap is not cleaned up, but that the LRUMap contains keys from another class-loader and with this keeps alive the other classloader until jackson's classloader is cleaned up -- which does not happen at runtime, you have to restart JBoss for that.

[pjfanning]

Maybe a clear method on the Annotation inspector could allow users to clear up that old data

[cowtowncoder]

Ah. Ok now it all makes sense. Series of unfortunate events, unintended side effects. So it's due to 2 things:

1. Use of Class<?>es from wherever (annotation types) as keys AND
2. Static reference to default introspector instance

Although (2) might be initially easier, I think (1) might actually make more sense: without resolving that there would still be possibility of retention via non-static references. Use of arbitrary Class instances as keys tends to lead to hard to track down retention.

A simple way that I'll probably is to use String of class name as the key. There's theoretical concern of same class via different class loaders, but I'd think these should refer to same definition (at leas wrt existence of @JacksonAnnotationsInside).

Unfortunately fix needs to go in 2.15.0, I think, just due to risk involved (no API change needed).
But I think it's otherwise fairly straight-forward; will tackle this today.

[pjfanning]

@cowtowncoder there is com.fasterxml.jackson.databind.type.ClassKey which seems to have been written for this purpose. But it might keep unwanted references too. It might be worth reviewing if ClassKey needs to be rewritten too - to drop the Class<?> refs and only keep the name.

[cowtowncoder]

@pjfanning That's something to consider too I suppose, although its actually usage should be less problematic as lookup maps are bound to individual ObjectMappers and there are no static singletons.

Having said that.... I am starting to think that maybe AnnotationIntrospector static instance really should go too. It being stateful makes that a bad idea.
So perhaps I'll do both (1) and (2).

[cowtowncoder]

Ok, not so fast I guess. The problem is not just ObjectMapper.DEFAULT_ANNOTATION_INTROSPECTOR but rather that it is then used in similarly shared BaseSettings.

What I'll do is create separate issue for follow-up work to do that refactoring.

[cowtowncoder]

Created #3772 for follow up work. Also starting to think this might be one of bad speculative optimizations, overall, given that without this JacksonAnnotationIntrospector is nicely stateless, reusable. And that there are no measurements to really show big benefits (theoretically I can think of kinds of set ups it might... but, theoretically).