Java: remove leak of JNI weak references in JniWrapperCache #1648
+116
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pwrobeldev
changed the title
|
Note to reviewers: the CI failed for the first commit, which introduced the stress test. This shows, that weak reference handles were leaked. Then a fix was added to PR. |
pwrobeldev
commented
Jan 29, 2025
gluecodium/src/main/resources/templates/jni/utils/JniWrapperCacheImplementation.mustache
Outdated
Show resolved
Hide resolved
Hsilgos
reviewed
Jan 31, 2025
| @@ -19,6 +19,10 @@ cmake_minimum_required(VERSION 3.6) | |||
|
|
|||
| project(functional) | |||
|
|
|||
| if (INTERNAL_GLUECODIUM_JNI_WEAK_REF_CHECK) | |||
| add_definitions(-DINTERNAL_GLUECODIUM_JNI_WEAK_REF_CHECK) | |||
There was a problem hiding this comment.
- Better use
GLUECODIUM_prefix:GLUECODIUM_INTERNAL_JNI_WEAK_REF_CHECK - Perhaps it can be more general name to combine it with more debug checks in the future.
add_definitionis obsolete function.target_compile_definitionsshould be used instead.- By good this option must be part of gluecodium-cmake scripts.
There was a problem hiding this comment.
Sure, fixed. Added a new optional property called GLUECODIUM_ENABLE_INTERNAL_DEBUG_CHECKS.
|
|
||
| #ifdef INTERNAL_GLUECODIUM_JNI_WEAK_REF_CHECK | ||
| if (s_wrapper_cache.count(obj_ptr) > 0) { | ||
| jenv->ThrowNew(jenv->FindClass("java/lang/Exception"), "Weak reference leaked!"); |
There was a problem hiding this comment.
I think you may call throw_new_runtime_exception(jenv, "Weak reference leaked!") ?
|
|
||
| #ifdef INTERNAL_GLUECODIUM_JNI_WEAK_REF_CHECK | ||
| if (iter == s_wrapper_cache.end()) { | ||
| jenv->ThrowNew(jenv->FindClass("java/lang/Exception"), "Invalid removal of cache entry"); |
| @@ -34,9 +36,17 @@ namespace jni | |||
|
|
|||
| static std::mutex s_mutex; | |||
| static std::unordered_map<const void*, jobject> s_wrapper_cache; | |||
| static std::unordered_map<const void*, int> s_skip_removals; | |||
There was a problem hiding this comment.
May be instead of 2nd global container we may keep jobject and "scheduled removals" in struct in s_wrapper_cache?
struct CachedObject {
jobject obj = nullptr;
unsigned int scheduled_removals = 0;
};
static std::unordered_map<const void*, CachedObject> s_wrapper_cache;
cc8665f to
a0278da
Compare
According to Java documentation of 'PhantomReference' class, which is used to schedule post-mortem cleanup of resources of C++ objects tied with Java objects and cleanup of the JniWrapperCache entry, there can be a time gap between clearing PhantomReference of Java object and putting PhantomReference object in the registered queue. During this time window the user of JniWrapperCache should not be capable of getting Java object from weak reference handle stored in wrapper cache map, because JNI weak reference returns null -- it is treated like PhantomReference and also cleared by GC. Sadly, the function, which adds to JniWrapperCache does not check if there is an entry for a given native pointer. As a result, we may override existing weak reference handle -- therefore, we would never destroy it. This results in a weak reference handle leak. The situation may happen if we query the same object multiple times. For instance when we have a C++ class, which has a member of object type that can be obtained via getter function. Calling this getter multiple times may cause weak reference handles leak. This commit brings a new functional test, which triggers the leak. Signed-off-by: Patryk Wrobel <[email protected]>
When trying to obtain a Java object from cache we may encounter a situation in which we cannot create local reference, because the Java object was garbage collected. In such a case obtained local reference points to NULL. We should delete weak reference and the entry from cache, because if the check is performed in the 'convert_to_jni()' function we create a new Java object and try to cache it in JniWrapperCache. The function which adds to cache does not check if it contains any record for given pointer. We may encounter the situation in which we override handle stored in cache without deleting it. This would lead to weak reference handle leak. Signed-off-by: Patryk Wrobel <[email protected]>
Signed-off-by: Patryk Wrobel <[email protected]>
This change extends debug checks in JniWrapperCache class to show the problem related to double dispose. When we remove an entry from JniWrapperCache in 'get_cached_wrapper_impl()' because the reference was invalidated we must anticipate, that the cleanup will be scheduled for this non-existent object. It may cause problems, because the cleanup of weak reference entry for the old object may remove the entry for the new entry. This commit extends checking and again makes 'stressTestJniWeakReferenceCache()' test to fail. In next commit the fix will be implemented to prevent such double-dispose. Signed-off-by: Patryk Wrobel <[email protected]>
When garbage collector clears the reference and we identify it in 'JniWrapperCache::cache_wrapper_impl()' we remove the entry from cache. This ensures, that references are not leaked in 'convert_to_jni()' function. However, this removal of entry from cache had very bad consequence. For the already cleared object disposal of cached entry can be scheduled after new entry for the same pointer is added to cache. In such situation the disposal scheduled for old object removed entry related to new object. In order to prevent that a counter was introduced. The last scheduled removal from cache erases the entry -- the remaining ones do not do anything. Signed-off-by: Patryk Wrobel <[email protected]>
pwrobeldev
force-pushed
the
pwrobeldev/jni-weak-references-leak
branch
from
a0278da to
116cdc4
Compare
Hsilgos
approved these changes
Feb 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
----- Motivation -----
According to Java documentation of 'PhantomReference' class, which is used to
schedule post-mortem cleanup of resources of C++ objects tied with Java objects
and cleanup of the JniWrapperCache entry, there can be a time gap between clearing
PhantomReferenceof Java object and putting PhantomReference object in the registeredqueue.
During this time window the user of
JniWrapperCacheshould not be capable of gettingJava object from weak reference handle stored in wrapper cache map, because JNI weak
reference returns null -- it is treated like
PhantomReferenceand also cleared by GC.Sadly, the function, which adds to JniWrapperCache does not check if there is an entry for a given
native pointer. As a result, we may override existing weak reference handle -- therefore, we would
never destroy it. This results in a weak reference handle leak.
The situation may happen if we query the same object multiple times. For instance when we have a
C++ class, which has a member of object type that can be obtained via getter function. Calling this
getter multiple times may cause weak reference handles leak.
----- Implemented changes -----