Skip to content
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

netty-jni-util is incompatible with static compilation #5

Closed
ejona86 opened this issue Mar 17, 2021 · 3 comments · Fixed by #6
Closed

netty-jni-util is incompatible with static compilation #5

ejona86 opened this issue Mar 17, 2021 · 3 comments · Fixed by #6

Comments

@ejona86
Copy link
Member

ejona86 commented Mar 17, 2021

When building statically there is only one copy of netty-jni-util. This means 1) all parts of the binary must use the same version of netty-jni-util and 2) static variables are shared. This is an issue if epoll and tcnative both are in the same static binary.

(1) introduces a bit of a theoretical problem. Since there is no API stability for netty-jni-util we need to make sure that both netty and netty-tcnative releases use the same version of netty-jni-util. Or there's weak enough stability to allow a little version skew between the projects (preferred, isn't always necessary). Either way this is probably easy and close to what might be done anyway, but didn't seem guaranteed.

(2) means that either the static variables must be shareable or they must be defined by each consumer of netty-jni-util instead. staticPackagePrefix is not shareable.

Would it be better to define staticPackagePrefix in the callers (maybe exposed as one member of a struct) and have it be passed by address to Load/Unload to be maintained by netty-jni-util? Or would it be better to just have the load_function() do a strcpy at the end of load_function() and store in its own static?

@normanmaurer
Copy link
Member

@ejona86 I think an strcpy might be "easier" ... WDYT ?

ejona86 added a commit to ejona86/netty-jni-util that referenced this issue Mar 24, 2021
Motivation:

When statically compiled, the same netty-jni-util may be used by
multiple separate JNI libraries. In this situation there is only one
copy of static variables, so it is only safe to use static variables in
cases where they can safely be reused. That's not the case for the
current staticPackagePrefix.

Modifications:

Force the caller to manage the static state, which can be unique per
usage.

Result:

Static compilation is available again for JNI libraries that supported
static compilation before depending on netty-jni-util. Fixes netty#5.
@ejona86
Copy link
Member Author

ejona86 commented Mar 24, 2021

SGTM. Sent out #6.

If you do a release of netty-jni-util, I can send a PR for netty-tcnative (I have the changes ready). I don't have a Mac, so it'd be a bit easier if you manage netty, although I can leverage the CI as a slow compiler in a pinch. Epoll wasn't too hard:

diff --git a/transport-native-epoll/src/main/c/netty_epoll_native.c b/transport-native-epoll/src/main/c/netty_epoll_native.c
index 4c1852f4f3..dc7118f6e3 100644
--- a/transport-native-epoll/src/main/c/netty_epoll_native.c
+++ b/transport-native-epoll/src/main/c/netty_epoll_native.c
@@ -722,13 +722,17 @@ done:
     return ret;
 }
 
-static void netty_epoll_native_JNI_OnUnload(JNIEnv* env, const char* packagePrefix) {
-    netty_epoll_linuxsocket_JNI_OnUnLoad(env, packagePrefix);
+static void netty_epoll_native_JNI_OnUnload(JNIEnv* env) {
+    netty_epoll_linuxsocket_JNI_OnUnLoad(env, staticPackagePrefix);
 
     if (register_unix_called == 1) {
         register_unix_called = 0;
         netty_unix_unregister(env, staticPackagePrefix);
     }
+
+    netty_jni_util_unregister_natives(env, staticPackagePrefix, STATICALLY_CLASSNAME);
+    netty_jni_util_unregister_natives(env, staticPackagePrefix, NATIVE_CLASSNAME);
+
     if (staticPackagePrefix != NULL) {
         free((void *) staticPackagePrefix);
         staticPackagePrefix = NULL;
@@ -741,9 +745,6 @@ static void netty_epoll_native_JNI_OnUnload(JNIEnv* env, const char* packagePref
     packetPortFieldId = NULL;
     packetMemoryAddressFieldId = NULL;
     packetCountFieldId = NULL;
-
-    netty_jni_util_unregister_natives(env, packagePrefix, STATICALLY_CLASSNAME);
-    netty_jni_util_unregister_natives(env, packagePrefix, NATIVE_CLASSNAME);
 }
 
 // Invoked by the JVM when statically linked

normanmaurer pushed a commit that referenced this issue Mar 24, 2021
Motivation:

When statically compiled, the same netty-jni-util may be used by
multiple separate JNI libraries. In this situation there is only one
copy of static variables, so it is only safe to use static variables in
cases where they can safely be reused. That's not the case for the
current staticPackagePrefix.

Modifications:

Force the caller to manage the static state, which can be unique per
usage.

Result:

Static compilation is available again for JNI libraries that supported
static compilation before depending on netty-jni-util. Fixes #5.
@normanmaurer
Copy link
Member

@ejona86 no worries I am on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants