New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The static field ClassUtil.sCached can cause a class loader leak #1363

Closed
stuartwdouglas opened this Issue Sep 5, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@stuartwdouglas
Contributor

stuartwdouglas commented Sep 5, 2016

The static field ClassUtil.sCached was added in 2.7 as a performance optimization, however as it can hold on to classes in a static field this can cause a class loader leak in application servers where databind is provided as part of the app server and not bundled in the deployment.

e.g.: https://issues.jboss.org/browse/WFLY-7037

@stuartwdouglas

This comment has been minimized.

Show comment
Hide comment
@stuartwdouglas

stuartwdouglas Sep 5, 2016

Contributor

Looks like com.fasterxml.jackson.databind.type.TypeFactory#_typeCache has a similar issue

Contributor

stuartwdouglas commented Sep 5, 2016

Looks like com.fasterxml.jackson.databind.type.TypeFactory#_typeCache has a similar issue

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Sep 6, 2016

Member

Yes, I can see how ClassUtil.sCached could be problematic in such a case. I'll have to think about it a bit: while use of soft/weak reference could help, it would basically render it useless on Android as far as I know (since VM doesn't really support these in a way to make them useful). So perhaps it'd just be best to remove it.

Potential problem with TypeFactory is more difficult, as it is not directly due to static member (_typeCached is non-static member), nor is type factor itself accessed statically.
Unfortunately... there is static "default" instance of TypeFactory itself, which makes it likely there is a similar problem. And this cache is quite essential for performance, much more so than metadata ClassUtil tries to cache.

Thank you for reporting this issue.

Member

cowtowncoder commented Sep 6, 2016

Yes, I can see how ClassUtil.sCached could be problematic in such a case. I'll have to think about it a bit: while use of soft/weak reference could help, it would basically render it useless on Android as far as I know (since VM doesn't really support these in a way to make them useful). So perhaps it'd just be best to remove it.

Potential problem with TypeFactory is more difficult, as it is not directly due to static member (_typeCached is non-static member), nor is type factor itself accessed statically.
Unfortunately... there is static "default" instance of TypeFactory itself, which makes it likely there is a similar problem. And this cache is quite essential for performance, much more so than metadata ClassUtil tries to cache.

Thank you for reporting this issue.

@stuartwdouglas

This comment has been minimized.

Show comment
Hide comment
@stuartwdouglas

stuartwdouglas Sep 6, 2016

Contributor

I have hacked up this ugly thing to work around it in the meantime: wildfly/wildfly#9173

Basically I just clear the caches via reflection when something is undeployed. One possibility for working around this could be introducing some kind of SPI to allow the container to clear any static caches.

Contributor

stuartwdouglas commented Sep 6, 2016

I have hacked up this ugly thing to work around it in the meantime: wildfly/wildfly#9173

Basically I just clear the caches via reflection when something is undeployed. One possibility for working around this could be introducing some kind of SPI to allow the container to clear any static caches.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Sep 6, 2016

Member

@stuartwdouglas yes, that might make sense as a short-term work-around until cleaner solution is found. I could and probably should add a static method in ClassUtil for clearing cached data. And to limit surface area for the work-around, that should probably then also trigger clean up of the default TypeFactory, so that only one call is needed. When handling is fixed in future, this method could become a no-op.

Member

cowtowncoder commented Sep 6, 2016

@stuartwdouglas yes, that might make sense as a short-term work-around until cleaner solution is found. I could and probably should add a static method in ClassUtil for clearing cached data. And to limit surface area for the work-around, that should probably then also trigger clean up of the default TypeFactory, so that only one call is needed. When handling is fixed in future, this method could become a no-op.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Sep 15, 2016

Member

I went ahead and ripped out the whole caching: while it could probably be made to work with WeakHashMap (although... might have issue wrt. value having reference...), this was mostly useful in pathological case of constructing new ObjectMapper for each call.

As to TypeFactory: although it would be nice to figure out how to avoid relying on static singleton default instance (and always create new instance for new ObjectMapper) -- and I'll create a new issue for that -- I think for now what has to suffice is existing (since 2.4) method of TypeFactory.clearCache(), called f.ex as:

TypeFactory.defaultInstance().clearCache()

which will clear cached entries from the shared default instance.

Member

cowtowncoder commented Sep 15, 2016

I went ahead and ripped out the whole caching: while it could probably be made to work with WeakHashMap (although... might have issue wrt. value having reference...), this was mostly useful in pathological case of constructing new ObjectMapper for each call.

As to TypeFactory: although it would be nice to figure out how to avoid relying on static singleton default instance (and always create new instance for new ObjectMapper) -- and I'll create a new issue for that -- I think for now what has to suffice is existing (since 2.4) method of TypeFactory.clearCache(), called f.ex as:

TypeFactory.defaultInstance().clearCache()

which will clear cached entries from the shared default instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment