Consolidate jcall to enable extension of JVM types

[ahnlabb]

The main purpose of this PR is to make it easier to extend JavaCall with types that e.g. need special cleanup, define a signature method, etc. (see #144).

Since the relevant logic to handle the core call through JNI was spread out, discrepancies had crept into the codebase:

https://github.com/JuliaInterop/JavaCall.jl/blob/7973dc760fc8b3fdd07baf763cf27b515d199e88/src/core.jl#L485

https://github.com/JuliaInterop/JavaCall.jl/blob/7973dc760fc8b3fdd07baf763cf27b515d199e88/src/core.jl#L458

So:

julia> jcall(JavaObject{Symbol("java.util.HashMap")}(C_NULL), "toString", JString, ())
ERROR: Attempt to call method on Java NULL
Stacktrace:
...

julia> jcall(JavaObject{Symbol("java.util.HashMap")}(C_NULL), "isEmpty", jboolean, ())
ERROR: JavaCall.JavaCallError("Attempt to call method on Java NULL")
Stacktrace:
...

I believe this discrepancy arose because the central logic for setting up and "tearing down" a call (including error checking) is fragmented. For this reason I don't feel comfortable adding the functionality in #144 before this logic has been consolidated e.g. through this PR.

My goal in this PR is therefore to more or less only have one place in core.jl where we:

• Convert arguments
• Use GC.@preserve
• Tear down temporary objects
• Check errors
• Convert result

for a Java method call.

[mkitti]

Tests seems to be failing with the latest push

[ahnlabb]

The expanded test suite from 7cc0fbf now captures the loss of the two-argument version of jfield. While working on the tests I noticed that exceptions are not thrown in julia unless a null pointer is returned, this is captured in the exceptions_1 test suite. Can you confirm that this is a bug @mkitti? If the errors are not checked after the initial call it they can surface at a later call which is not actually the call that threw the exception, you can reproduce it by the following (using the Test class from the test directory):

julia> JTest = @jimport(Test)
JavaObject{:Test}

julia> t=JTest(())
JavaObject{:Test}(JavaLocalRef(Ptr{Nothing} @0x000000000210a898))

julia> jcall((@jimport java.lang.Math), "floorDiv", jint, (jint, jint), 1, 0)
0

julia> jfield(t, "objectField", JObject)
Exception in thread "main" java.lang.ArithmeticException: / by zero
        at java.lang.Math.floorDiv(Math.java:1052)
ERROR: JavaCall.JavaCallError("Error calling Java: java.lang.ArithmeticException: / by zero")
Stacktrace:
 [1] geterror(allow::Bool)
   @ JavaCall ~/projects/JavaCall.jl/src/core.jl:537
 [2] geterror
   @ ~/projects/JavaCall.jl/src/core.jl:522 [inlined]
 [3] _jfield(obj::JavaObject{:Test}, jfieldID::Ptr{Nothing}, fieldType::Type)
   @ JavaCall ~/projects/JavaCall.jl/src/core.jl:435
 [4] jfield(obj::JavaObject{:Test}, field::String, fieldType::Type)
   @ JavaCall ~/projects/JavaCall.jl/src/core.jl:398
 [5] top-level scope
   @ REPL[12]:1

Expanded test suite:

7cc0fbf (master with new test suite):

JavaCall                     |  212     4      4    220
  unsafe_strings_1           |    3                   3
  parameter_passing_1        |   13                  13
  static_method_call_1       |    3                   3
  static_method_call_async_1 |                    No tests
  instance_methods_1         |    3                   3
  exceptions_1               |          3      3      6
  fields_1                   |   20            1     21
    UInt8                    |    3                   3
    Int32                    |    3                   3
    JString                  |    3                   3
    JObject                  |    3                   3
  null_1                     |    9     1            10
  arrays_1                   |   14                  14
  dates_1                    |    6                   6
  map_conversion_1           |    1                   1
  array_list_conversion_1    |    1                   1
  inner_classes_1            |    4                   4
  sinx_1                     |    2                   2
  method_lists_1             |    9                   9
  double_free_1              |  100                 100
  array_conversions_1        |    1                   1
  iterator_conversions_1     |   11                  11
  roottask_and_env_1         |    4                   4
  jlocalframe                |    8                   8

In addition to the issue with exceptions mentioned above the new tests also caught a typo on master ("Bype"): https://github.com/JuliaInterop/JavaCall.jl/blob/7973dc760fc8b3fdd07baf763cf27b515d199e88/src/core.jl#L414 I'll fix it in this PR. EDIT: Or rather it is already fixed because the consolidation of the logic avoids the repetition that caused this typo.

3a8248e (this PR)

Test Summary:                | Pass  Fail  Error  Total
JavaCall                     |  206     3     11    220
  unsafe_strings_1           |    3                   3
  parameter_passing_1        |   13                  13
  static_method_call_1       |    3                   3
  static_method_call_async_1 |                    No tests
  instance_methods_1         |    3                   3
  exceptions_1               |          3      3      6
  fields_1                   |   13            8     21
    UInt8                    |    1            2      3
    Int32                    |    1            2      3
    JString                  |    1            2      3
    JObject                  |    1            2      3
  null_1                     |   10                  10
  arrays_1                   |   14                  14
  dates_1                    |    6                   6
  map_conversion_1           |    1                   1
  array_list_conversion_1    |    1                   1
  inner_classes_1            |    4                   4
  sinx_1                     |    2                   2
  method_lists_1             |    9                   9
  double_free_1              |  100                 100
  array_conversions_1        |    1                   1
  iterator_conversions_1     |   11                  11
  roottask_and_env_1         |    4                   4
  jlocalframe                |    8                   8

[mkitti]

I'm not able to investigate myself at the moment, but that certainly looks like a bug to me.

[mkitti]

isnothing is not implemented in Julia 1.0. Just use === nothing instead.

[ahnlabb]

Thank you @mkitti. It looks like the checks are passing now. It is important to note that this PR now includes some important changes to errors. As noted above:

While working on the tests I noticed that exceptions are not thrown in julia unless a null pointer is returned

Not only were the exceptions not thrown but they were not cleared meaning that a latter call returning a valid null-pointer would throw. When fixing this issue I noticed another related one. Null checks that did not properly use the exception checking mechanism of the JNI assumed a specific reason for the returned null even when several possibilities existed. The chosen reason was probably the most common one but this handling could sometimes obscure valuable information about the real error. This could happen when trying to get a constructor or find a class:

JNI.GetMethodID will according to the spec return a null pointer in the following situations:

NoSuchMethodError: if the specified method cannot be found.

ExceptionInInitializerError: if the class initializer fails due to an exception.

OutOfMemoryError: if the system runs out of memory.

However, on the current master, we throw: JavaCallError("No constructor for $T with signature $sig") when using JNI.GetMethodID to get the constructor for all situations without checking, clearing, or reporting the error.

Similarly:

JNI.FindClass will return a null pointer in these situations:

ClassFormatError: if the class data does not specify a valid class.

ClassCircularityError: if a class or interface would be its own superclass or superinterface.

NoClassDefFoundError: if no definition for a requested class or interface can be found.

OutOfMemoryError: if the system runs out of memory.

And we throw: JavaCallError("Class Not Found $jclass").

In this PR I instead handle all possible exceptions using geterror which provides the exception as reported by the JVM and clears the exception. For example, if we create a corrupted class file TestBad.class (e.g. by deleting som parts of Test.class:

master:

julia> JavaCall.jnew(Symbol("TestBad"))
ERROR: JavaCall.JavaCallError("Class Not Found TestBad")
Stacktrace:
 [1] _metaclass(class::Symbol)
   @ JavaCall JavaCall.jl/src/core.jl:501
[...]

julia> JavaCall.jnew(Symbol("TestNonexistentClassfile"))
ERROR: JavaCall.JavaCallError("Class Not Found TestNonexistentClassfile")
Stacktrace:
 [1] _metaclass(class::Symbol)
   @ JavaCall JavaCall.jl/src/core.jl:501
[...]

ahnlabb:consolidate-jcall:

julia> JavaCall.jnew(Symbol("TestBad"))
Exception in thread "main" java.lang.ClassFormatError: Unknown constant tag 68 in class file TestBad
        at java.lang.ClassLoader.defineClass1(Native Method)
        at java.lang.ClassLoader.defineClass(ClassLoader.java:756)
        at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
        at java.net.URLClassLoader.defineClass(URLClassLoader.java:468)
        at java.net.URLClassLoader.access$100(URLClassLoader.java:74)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:369)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
ERROR: JavaCall.JavaCallError("Error calling Java: java.lang.ClassFormatError: Unknown constant tag 68 in class file TestBad")
Stacktrace:
 [1] geterror()
   @ JavaCall JavaCall.jl/src/core.jl:487
[...]

julia> JavaCall.jnew(Symbol("TestNonexistentClassfile"))
Exception in thread "main" java.lang.NoClassDefFoundError: TestNonexistentClassfile
Caused by: java.lang.ClassNotFoundException: TestNonexistentClassfile
        at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
ERROR: JavaCall.JavaCallError("Error calling Java: java.lang.NoClassDefFoundError: TestNonexistentClassfile")
Stacktrace:
 [1] geterror()
   @ JavaCall JavaCall.jl/src/core.jl:487
[...]

In summary. In addition to the previously discussed changes this PR does two new things:

1. Fixes a bug where sometimes an exception was not thrown and/or not cleared.
2. Removes to special cases of error handling in favor of the general and more correct error handling of geterror

Change 1 should be uncontroversial. While change 2 results in the correct error being shown it can in some cases result in a less "user-friendly" error. I think a lot could be done to make the errors more user-friendly and easier to interact with. A possible way forward would be to merge this PR and then open a new PR or issue where this could be discussed further, what do you think @mkitti?

[mkitti]

I think a lot could be done to make the errors more user-friendly and easier to interact with. A possible way forward would be to merge this PR and then open a new PR or issue where this could be discussed further.

Let's build some infrastructure here for this around the checknull function. An optional second argument would help us annotate the reason for the null. An additional @checknull macro might help us automatically capture the calling function, which we could then relay to checknull as an optional third argument.

[ahnlabb]

Thank you @mkitti for the in-depth review. I may not have time to got through everything today but for the general comments:

Could you justify dropping the type annotations? I tend to be more specific about them in core packages such as this.

We can definitely put in some type annotations. I don't do it by default and prefer to be explicit about their purpose. For example:

function jcall(ref, method::AbstractString, rettype::Type, argtypes::Tuple = (), args...)
    assertroottask_or_goodenv() && assertloaded()
    jmethodId = checknull(get_method_id(ref, method, rettype, argtypes))
    _jcall(_jcallable(ref), jmethodId, rettype, argtypes, args...)
end

From a library maintainer/readability perspective, to me, this defines a very clear contract and I would actually prefer removing the AbstractString annotation. Calling jcall requires:

• The JVM to be initialized
• ref and method to be types that have a JavaCall.get_method_id method.
• ref to be a type with a _jcallable method

If I want to make a new type (like native arrays) with a jcall method all I need is to implement _jcallable and get_method_id. We quickly dispatch to more specialized functions, there is probably no performance benefit to annotating. The more specialized methods yield errors that indicate that the contract has been broken. For a core library like this I think it's important to enable other libraries to extend in ways we may not expect:

julia> jcall("abc.abc", "toUpperCase", JString)
ERROR: MethodError: no method matching get_method_id(::String, ::String, ::Type{JString}, ::Tuple{})
Closest candidates are:
  get_method_id(::Type{JavaObject{T}}, ::AbstractString, ::Type, ::Tuple) where T at /home/ja/projects/JavaCall.jl/src/core.jl:356
  get_method_id(::JavaObject, ::AbstractString, ::Type, ::Tuple) at /home/ja/projects/JavaCall.jl/src/core.jl:360
  get_method_id(::Any, ::Any, ::AbstractString, ::Type, ::Tuple) at /home/ja/projects/JavaCall.jl/src/core.jl:351
Stacktrace:
[...]

julia> JavaCall.get_method_id(str::String, args...) = JavaCall.get_method_id(JavaCall.JNI.GetMethodID, Ptr(JavaCall.metaclass(JString)), args...)

julia> JavaCall._jcallable(str::String) = JString(str)

julia> jcall("abc.abc", "toUpperCase", JString)
"ABC.ABC"

The disadvantage, as you indicate, is that we may lose some documentation from a library user's perspective. For example typing jcall(<TAB> in the REPL yields very limited information. I agree that expanding documentation is probably necessary and some functions (like get_method_id(jnifun, ptr, method::AbstractString, rettype::Type, argtypes::Tuple)) may benefit from annotation.

checknull definitely seems to help simplify the code

Here's my suggestion for a @checknull macro

Looks great to me, I considered creating a similar macro but was also thinking that we perhaps could exhaustively enumerate the JNI spec exceptions and provide Julia types for them so that users (perhaps mostly library authors) could match on them. Alternatively, we could figure out a generic way to present java exceptions, perhaps making a symbol from the Throwable class. Perhaps that wouldn't be very useful or too much work but giving this some thought so we don't lock into a rigid approach could be good. When handling an error inside a library do I care more that it happened during a specific part of the call to the constructor or that it was an OOM exception? Can we allow handling of the exception in a simple way using either part of this information? I think we can probably do better than just a JavaCallError wrapping a string but perhaps you're right in suggesting we start with such a solution in this PR.

[mkitti]

About the error part, we probably should make a JavaError or JNIError that wraps a JavaObject which represents the underlying error. That would provide a mechanism to detect the error.

How are we doing on this pull request otherwise?

[mkitti]

@ahnlabb would you mind I contributed some commits to this branch?

[ahnlabb]

Hi @mkitti, I'm sorry that I've been unresponsive. I just went on vacation and needed to finalize some other things.

I understand that a lot is riding on this PR now and as I unfortunately won't have much time in front of a computer the coming two weeks I welcome contributions from your side.

I'll still be available for discussion!

[ahnlabb]

@mkitti did you continue work here (anything not pushed to the branch)? Could I devote some time to this PR the coming weeks?

[mkitti]

I have not made progress. Go ahead.

[mkitti]

This is about where I want it now. It could use some more documentation.

@aviks , any thoughts about merging this?

[ahnlabb]

@mkitti I've added the previously discussed simplification of the types as well as a small refactor that would make it easier to add something like this to geterror in the future:

function get_exception_object(jthrow)
    jclass = JNI.FindClass("java/lang/Class")
    _notnull_assert(jclass)

    thrown_class = JNI.GetObjectClass(jthrow)
    _notnull_assert(thrown_class)

    getname_method = JNI.GetMethodID(jclass, "getName", "()Ljava/lang/String;")
    _notnull_assert(getname_method)

    res = JNI.CallObjectMethodA(thrown_class, getname_method, Int[])
    _notnull_assert(res)

    return JavaObject{Symbol(unsafe_string(JString(res)))}(jthrow)
end

so that we can, as discussed, throw the exception as an object instead of a string. This would probably require ways to pick the desired behavior. I have thoughts but it would be a different PR.