Fix a bug in ConstructorCache when classes are GC'ed but not removed from cache #6186
Conversation
| if (classRef.isPresent() && classRef.get().get() == null) { | ||
| classesByClassLoader.remove(classLoader); | ||
| return getClass(className); | ||
| } | ||
| return classRef.map(WeakReference::get); |
There was a problem hiding this comment.
Is it still possible for the weak ref to get GC'd between 67 and 71? Would it be better to store classRef.get().get() to a temporary var to avoid that happening?
There was a problem hiding this comment.
Ooff - yeah, good catch - I've updated.
On a related note - I had been focused on checking that this was thread safe - and because of the usage of synchronized maps, I believe it is - in particular the classesByClassLoader.remove(classLoader) and classesByClassLoader.computeIfAbsent generally ensure the cache is consistent. There is an issue where a class could be removed and re-loaded multiple times which would result in extra work, but not in an inconsistent/incorrect cache state.
There was a problem hiding this comment.
Cool, yeah agree don't think there's an issue where the cache could get corrupted in that case
|
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
Fix a bug in ConstructorCache when classes are GC'ed but not removed from cache
Motivation and Context
The ConstructorCache uses WeakReferences to hold Class objects, which can be garbage-collected, leading to Optional.empty() results even though the class may still be load-able. The ConstructorCache is using a WeakHashMap which handles GC of the keys only, but not when values (the class Objects in this case) are cleaned up.
The current design of the constructor cache was introduced to store the classloader and class with a weak reference, so that we don't prevent them from being unloaded. It was intended to follow We do something similar in DynamoDB enhanced's BeanTableSchema but, unlike the BeanTableSchema, this cache also uses weakreferences to the values being cached.
Modifications
Adds an additional check in the
getClassmethod to check if the referent in the WeakReference has been garbage collected and if so, remove it from the cache.Q: Why not use a
ReferenceQueueto handle removals as in the implementation of WeakHashMap?Short answer: it increases complexity more than is justified to handle an edge case error. Long answer: The schema of the cache (specially
Map<String, Map<ClassLoader, Optional<WeakReference<Class<?>>>>>) makes this more difficult to track - we would need to create a new entry class that extends WeakReference and stores the className and classLoader and would need to ensure the reference to classLoader is also a weak reference in this entry. We would then need to add synchronization around check the reference queue and logic to remove cache entries from the nested cache correctly.Testing
Testing this edge case is extremely difficult - I was able to manually reproduce the issue with a hacked custom class loader + multiple threads, but its not consistently reproducible, so I have not added an automated unit test to cover this case.
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License