Refactor AbstractEventListener to cache Class instance in JNI code #12844
Conversation
rhubner
force-pushed
the
eb/callback-sigsegv-bug
branch
from
ff2fad2
to
a91ce90
Compare
rhubner
force-pushed
the
eb/callback-sigsegv-bug
branch
3 times, most recently
from
fbec614
to
7c96ed2
Compare
| * This exception should be used in situation when critical | ||
| * JNI call failed. For example GetJvm, FindClass, ... | ||
| */ | ||
| class JniException : public std::exception { |
There was a problem hiding this comment.
In a perfect world KVException would probably inherit from JniException - templates may be involved also :-( We should add a comment to link the two. And even a backlog ticket for the work ?
There was a problem hiding this comment.
Created card in backlog (3308)
| * It loads rocksDB code with custom classloader and then test that all | ||
| * event data for event listener can be instantiated. | ||
| */ | ||
| public class EventListenerClassloaderTest { |
There was a problem hiding this comment.
This has a bunch of check warnings when I open it in IntelliJ. I'm surprised checkbugs isn't complaining. Is it missing from a list ?
There was a problem hiding this comment.
I think checkbugs verify only production code, not tests.
I address most of the IntelliJ warning. Including wrong order on :
assertThat(jarPath).as("Java property 'rocks-jar' was not setup properly").isNotNull();
| + "classLoader" | ||
| + " to verify that callback cache all class instances."); | ||
| } catch (ClassNotFoundException e) { | ||
| ; |
There was a problem hiding this comment.
It's empty for purpose. We are testing if RocksDB is on class path. And success state is ClassNotFoundException. Not on default classpath. 🤔 Maybe if we can use somehow to translate this to assumptions org.junit.Assume 🤔.
| } | ||
|
|
||
| String jarPath = System.getProperty("rocks-jar"); | ||
| assertThat(jarPath).isNotNull().as("Java property 'rocks-jar' was not setup properly"); |
There was a problem hiding this comment.
result of as ignored - this assertThat isn't doing what you think it is
There was a problem hiding this comment.
Fixed. as must be exactly after assertThat
| assertThat(jarPath).isNotNull().as("Java property 'rocks-jar' was not setup properly"); | ||
|
|
||
| Path classesDir = Paths.get(jarPath); | ||
| ClassLoader cl = new URLClassLoader(new URL[] {classesDir.toAbsolutePath().toUri().toURL()}); |
There was a problem hiding this comment.
URLClassLoader is Closeable - try with resources needed
|
|
||
| Class testableEventListenerClazz = cl.loadClass("org.rocksdb.test.TestableEventListener"); | ||
| Method invokeAllCallbacksInThread = | ||
| testableEventListenerClazz.getMethod("invokeAllCallbacksInThread"); |
There was a problem hiding this comment.
Unchecked call getMethod
There was a problem hiding this comment.
Not a problem It will fail whole test with nice exception and stack trace. I think better that manual check.
java/rocksjni/portal.h
Outdated
|
|
||
| // StatusJni can't cache jclass because in descructor we need JniUtil, |
| // StatusJni can't cache jclass because in descructor we need JniUtil, | ||
| // JniUtil need RocksDBExceptionJni, and RocksDBExceptionJni need StatusJni. | ||
| // So I can't even change the order of the classes in portal.h | ||
| static jobject construct(JNIEnv* env, const Status& status, jclass jclazz, |
There was a problem hiding this comment.
Could you comment the method ? And explain the dependencies a bit more.
There was a problem hiding this comment.
Add proper documentation.
java/rocksjni/portal.h
Outdated
| auto status = env->GetJavaVM(vm); | ||
| if (status != 0) { | ||
| ROCKSDB_NAMESPACE::JniException::ThrowNew( | ||
| env, "Unable to retrieve instance ov JavaVM"); |
There was a problem hiding this comment.
should be "instance of JavaVM"
java/rocksjni/event_listener.cc
Outdated
| env, jobj, enabled_event_callbacks)); | ||
| return GET_CPLUSPLUS_POINTER(sptr_event_listener); | ||
| } catch (ROCKSDB_NAMESPACE::JniException&) { | ||
| // We always throw JniException with Java Exception |
There was a problem hiding this comment.
I would say:
// JNIException indicates that a Java Exception has been thrown in the env
|
LGTM now. Happy for you to un-draft it. |
Move AbstractEventListenerJni to separate file to remove cyclic dependency Refactor portal classes related to eveent litener to cache class and methodID.
rhubner
force-pushed
the
eb/callback-sigsegv-bug
branch
from
d93be33
to
e2377b8
Compare
Problem:
NoClassDefFoundErrorthrown from JNI code whenEventListeneris setup.Condition to replicate :
RocksDB jar must be loaded with a custom class loader, for example Web container, Spring boot, …
Cause:
Limitation of java class loader in JNI API. JNI code is able to load classes from the same custom class loader only when the call to JNI was initiated from Java and from class loaded with the same class loader. Unfortunately in the callback, the call is initiated by RocksDB and then we attach to the existing JVM. In this situation, limitation of
FindClassmethod takes place and JVM is able to only load classes which are specified on start of JVM.Solution:
To overcome this limitation, in
EventListenerJniCallbackconstructor JNI code, we load all instances ofjava.lang.Classrequired later by the callback and make them global references. Because in this phase, JNI code is called from Java, JVM knows where to look for these classes. Then later we are able to create instances of these classes, even when the JNI code wasn’t initiated from Java code and the current thread was attached to JVM.This PR fix #10686