Get JNIEnv pointer

[ncruces]

Suggested fix for #805. Implemented on top of #811 (related).

Changes the native side (updates version and hash). Includes a test case.

Hope its up to standard.

[matthiasblaesing]

@ncruces if I read the referenced issue correctly, you are not interested in the JNIEnv Pointer on the java side, but on the native side. I see an alternative:

• introduce a marker class (something along the lines of:

public final class CurrentJNIEnv {
    public static CurrentJNIEnv INSTANCE = new CurrentJNIEnv();
}

• Specialcase this class on the native side to be replaced with the env pointer (see dispatch function in dispatch.c)

This way the Java->Native transition needs to be done only once and you are sure to access the "correct" JNI env.

As a general comment, if the native interface is changed, the changes done to the lib/native directory need to be commited with the change, as the build skript takes care of invalidating the native libraries, that don't work anymore.

@twall Timothy could you please also have a look at this?

[ncruces]

Yes, that's a possibility. It'd be cleaner for client code. But it's a bigger change and makes everyone else pay a (small) price on every dispatch for something I imagine isn't that common.

Right now I'm caching the environment pointer on my end as a thread local static, so I only pay the price once per thread. Then, it's a non issue for me.

But I can sketch that alternative and do a separate PR for you to decide which is better? I'd need to change direct dispatch as well.

[ncruces]

Sorry for that.

About binaries, I can do those, but I don't think I can for every applicable platform? I've done Linux x86-64 and all the Androids.

[matthiasblaesing]

With regard for the binaries: I can do the currently supported linux variants and windows. I have a way to build the darwin binary and solaris x64. Timothy should be able to do the AIX build. Long story short: building the binaries is a doable job.

My fear with your current approach is users using the wrong JNIEnv pointer and not taking the same care as you. Having the environment provide the "correct" JNIEnv removes that option to shoot onself into the foot.

[ncruces]

OK. We agree on that one. Most non-hackish uses of JNIEnv should be calling a native function that expects it (along with some jobjects). For the more hackish uses there's always my gist.

I'll have a go at it then.

Do you want me to base this work on the current branch, or on 5.0.0?

[ncruces]

OK, so this was easier than I thought: 0c3bc06

It passes the tests, and works for me. Any feedback is appreciated.
I can create a separate PR for it if you want to see Travis output.

I imagine jni.minor needs to be changed in build.xml before merging (unless this change is bundled with something else that already does that).
I haven't checked in binaries (again) but will do so for the platforms I'm building for once you guys are ready to accept a PR.

I've also run into some probable bugs (and improvements) when going through the source code. I'll create separate issues/branches for each of those.

[twall]

Handling of JNIEnv was one of the original use cases for the "allow objects" flag. This looks like a good implementation for ensuring the proper env is used. Good thinking, @matthiasblaesing

Thanks for the contributions, @ncruces

[matthiasblaesing]

@ncruces I did some more reading through the code and while going through the changes for supporting direct mapping of objects I noticed, that the unittest is not a real test (https://github.com/ncruces/jna/commit/86c21b8e971bbc8b3b744837bd0fad9ee30fd6f4). Your implementation of testReturnObject is basically a no-op. The change works nevertheless. I suggest a change along these lines:

diff --git a/test/com/sun/jna/DirectReturnTypesTest.java b/test/com/sun/jna/DirectReturnTypesTest.java
index afe0778..696f7a0 100644
--- a/test/com/sun/jna/DirectReturnTypesTest.java
+++ b/test/com/sun/jna/DirectReturnTypesTest.java
@@ -23,7 +23,6 @@
  */
 package com.sun.jna;
 
-import java.util.Map;
 import java.util.Collections;
 
 /** Exercise a range of native methods.
@@ -94,6 +93,7 @@ public class DirectReturnTypesTest extends ReturnTypesTest {
     @Override
     protected void setUp() {
         lib = new DirectTestLibrary();
+        libSupportingObject = new DirectObjectTestLibrary();
     }
 
     public static class DirectObjectTestLibrary extends DirectTestLibrary {
@@ -123,12 +123,6 @@ public class DirectReturnTypesTest extends ReturnTypesTest {
         return new DirectNativeMappedLibrary();
     }
 
-
-    @Override
-    public void testReturnObject() {
-        lib = new DirectObjectTestLibrary();
-    }
-
     // Override not-yet-supported tests
     @Override
     public void testReturnPointerArray() { }
diff --git a/test/com/sun/jna/ReturnTypesTest.java b/test/com/sun/jna/ReturnTypesTest.java
index 4a60ed8..2727181 100644
--- a/test/com/sun/jna/ReturnTypesTest.java
+++ b/test/com/sun/jna/ReturnTypesTest.java
@@ -129,26 +129,28 @@ public class ReturnTypesTest extends TestCase {
     }
 
     TestLibrary lib;
+    TestLibrary libSupportingObject;
     @Override
     protected void setUp() {
         lib = Native.loadLibrary("testlib", TestLibrary.class);
+        libSupportingObject = Native.loadLibrary("testlib", TestLibrary.class, Collections.singletonMap(Library.OPTION_ALLOW_OBJECTS, Boolean.TRUE));
     }
 
     @Override
     protected void tearDown() {
         lib = null;
+        libSupportingObject = null;
     }
 
     public void testReturnObject() throws Exception {
-        lib = Native.loadLibrary("testlib", TestLibrary.class, Collections.singletonMap(Library.OPTION_ALLOW_OBJECTS, Boolean.TRUE));
-        assertNull("null value not returned", lib.returnObjectArgument(null));
+        assertNull("null value not returned", libSupportingObject.returnObjectArgument(null));
         final Object VALUE = new Object() {
             @Override
             public String toString() {
                 return getName();
             }
         };
-        assertEquals("Wrong object returned", VALUE, lib.returnObjectArgument(VALUE));
+        assertEquals("Wrong object returned", VALUE, libSupportingObject.returnObjectArgument(VALUE));
     }
 
     public void testReturnObjectUnsupported() throws Exception {

One nice thing would be an explanation why native code does not need changing for direct passing of Java objects (CVT_OBJECT is not used on native side). I think I got it, but would be interested in your reasoning.

The changes for passing JNIEnv in 0c3bc06 look good to me.

I suggest this:

• recheck the patch for supporting java class in direct mapping
• ensure that you commit the changes to the native libraries in lib/native (not dist!). You will get a usable library for your changes and the other builds are replaced with out-of-date.jar, invalidating them
• rebase 0c3bc06 ontop of the changes/adjustments for direct mapping
• again ensure changes to lib/native are committed
• move the branch pointer get-env to the head of that changes
• force push to git

[matthiasblaesing]

One thing I forgot: If no api breaks are planned, I would base the changes on master. I created the jna-5.0.0 branch for changes that affect the API or backwards compatibility.

My plan is: Merge the changes done to master into the JNA 5.0.0 branch, so that this does not stop development.

[ncruces]

You are of course right about the test, I messed up. I've worked on that according to your suggestion, but changed it for the NativeMapped bits as well. OK?

Regarding CVT_OBJECT not being used in the native side, the reason is it jobject doesn't need any special handling on the native side. I needed just rtype/atype to be ffi_type_pointer here and here. This seemed the simplest way to achieve that. Also CVT_OBJECT is not the only one such constant not used on the native side. CVT_BOOLEAN/CVT_ARRAY_BOOLEAN aren't either. Are those bugs?

Regarding binaries, I'm not seeing out-of-date.jar invalidating platforms I cannot build. Maybe because the native ABI isn't changing (checksum is the same). Should I bump jni.minor, would that fix it?

[ncruces]

Not wanting to overwhelm you with low priority stuff, going down the CVT_ARRAY_BOOLEAN rabbit hole led me to #816.

CVT_BOOLEAN does seem unnecessary (as long as boolean is always mapped to ffi_type_uint32).

[matthiasblaesing]

@ncruces could you please have a look at this branch:

https://github.com/matthiasblaesing/jna/tree/ncruces-get-env

This is a partitial merge of your work. The changes I added:

• I skipped the native parts and for now just invalidated all native libraries (matthiasblaesing@a749072). I would have rebuild the linux part anyway (ensuring one baseline for the C library dependency)
• To make the unittest more stable the ELFAnalyserTest uses the dist binaries, not the lib/native ones (matthiasblaesing@bfcc246)

If this is ok with you I'd merge the into master. With regard to the new PR I would hold back on rebuilding the native parts until that is merged.

---

If your wondering about the automatic invalidation of the native libraries. For that to work the correct sequence is (this is only an information and no request for changes!):

1. Make the changes to the native interface
2. run the build (you will be informed, that the checksum changed, see the ant target -native-api-check)
3. Update the checksum as you did
4. Build native parts

After step 2 all binaries in lib/native' are replaced with the contents of out-of-date.jar`.

[ncruces]

That's fine by me. For now, I'm be using custom built binaries (so no pressure on you guys), but I'll gladly go back to Maven when this lands!

---

Regarding the invalidation of the native libraries: if I understand you correctly this means the invalidation only happens when the checksum changes (which is a proxy for a change in the native API). Correct?

The thing is: the solution we came too does not change the native API. Features were added, and the native part needs to be rebuilt for tests to pass, but the API doesn't change.

Maybe jni.minor needs to be bumped to reflect that?
I'm not sure, just trying to help.

[matthiasblaesing]

@ncruces You are right, the minor version needs to be bumped, as the changes you did, did not affect the JNI checksum, the automatic did not catch that - sorry about the confusion.