Consolidate jcall to enable extension of JVM types #149
Conversation
| end | ||
|
|
||
| function jfield(obj::JavaObject, field::JField) |
There was a problem hiding this comment.
Did we drop the two argument version of jfield?
There was a problem hiding this comment.
Good catch! I'm expanding the test suite to test for this as well as some other things.
ahnlabb
force-pushed
the
consolidate-jcall
branch
from
bd04093 to
3a8248e
Compare
|
Tests seems to be failing with the latest push |
|
The expanded test suite from 7cc0fbf now captures the loss of the two-argument version of Expanded test suite:7cc0fbf (master with new test suite): In addition to the issue with exceptions mentioned above the new tests also caught a typo on master ("Bype"): Line 414 in 7973dc7 3a8248e (this PR) |
|
I'm not able to investigate myself at the moment, but that certainly looks like a bug to me. |
ahnlabb
force-pushed
the
consolidate-jcall
branch
from
3a8248e to
0e4a3ac
Compare
|
|
ahnlabb
force-pushed
the
consolidate-jcall
branch
from
0e4a3ac to
ef31d84
Compare
|
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:
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:
However, on the current master, we throw: Similarly:
And we throw: In this PR I instead handle all possible exceptions using
In summary. In addition to the previously discussed changes this PR does two new things:
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? |
There was a problem hiding this comment.
I have gone through this with a finer comb.
- Could you justify dropping the type annotations? I tend to be more specific about them in core packages such as this.
checknulldefinitely seems to help simplify the code. It might be useful to have a second argument to add a note to the error if it is null. Perhaps this should be a macro so that we could elaborate on which JNI call created the error. Otherwise, as a function, consider adding an@inlineannotation.
| ptr | ||
| end | ||
|
|
||
| function geterror() |
There was a problem hiding this comment.
Do we need not the allow argument anymore?
There was a problem hiding this comment.
The allow argument is no longer needed. We have moved the decision whether to throw when no error is present to the caller. Basically, where we had geterror(true) we now have checknull().
| # Call static methods | ||
| function jcall(typ::Type{JavaObject{T}}, method::AbstractString, rettype::Type, argtypes::Tuple = (), | ||
| args... ) where T | ||
| function jcall(ref, method::AbstractString, rettype::Type, argtypes::Tuple = (), args...) |
There was a problem hiding this comment.
Should ref be Any like this or perhaps should it be a Union{Type{JavaObject}, JavaObject}? Do we need an AbstractJavaObject?
There was a problem hiding this comment.
I would prefer an AbstractJavaObject to a union. See the discussion in my general reply.
There was a problem hiding this comment.
With AbstractJavaObject it might be Union{Type{JavaObject}, AbstractJavaObject}
| # JMethod invoke | ||
| (m::JMethod)(obj, args...) = jcall(obj, m, args...) | ||
|
|
||
|
|
||
| function jfield(typ::Type{JavaObject{T}}, field::AbstractString, fieldType::Type) where T | ||
| function jfield(ref, field, fieldType) |
There was a problem hiding this comment.
I'm again wondering about dropping the type annotations. Part of it is the lack of documentation here. Perhaps we should improve the documentation while we are making these changes.
There was a problem hiding this comment.
I completely agree with the need for documentation. AbstractJavaObject is a possibility here as well.
| GC.@preserve savedArgs begin | ||
| result = callmethod(Ptr(obj), jmethodId, Array{JNI.jvalue}(jvalue.(convertedArgs))) | ||
| function _jcall(obj::T, jmethodId::Ptr{Nothing}, rettype::$x, | ||
| argtypes::Tuple, args...; callmethod=$callmethod) where T <: $t |
There was a problem hiding this comment.
Do we still need callmethod as an argument here? If so, do we still need to validate it?
There was a problem hiding this comment.
We don't need to check for C_NULL as was done previously, C_NULL was used as a special value to be able to set a default, much like a keyword argument or default argument.
The callmethod kwarg is only used by jnew. I think this is ok, alternatively, we could break it up into two functions (manually generate functions with defaults like what julia generates for us now with kwargs). This could be more performant. There is if I recall correctly a slight performance penalty to functions with keyword arguments.
|
|
||
| global const _jmc_cache = [ Dict{Symbol, JavaMetaClass}() ] | ||
|
|
||
| function _metaclass(class::Symbol) | ||
| jclass=javaclassname(class) | ||
| jclassptr = JNI.FindClass(jclass) | ||
| jclassptr == C_NULL && throw(JavaCallError("Class Not Found $jclass")) |
There was a problem hiding this comment.
We're losing the the specificity of the error message here. We should add a second argument to checknull for when we can provide a more descriptive error.
There was a problem hiding this comment.
Yes, I agree.
It is important to note that, as noted above, the error message can be wrong (or at the very least misleading). If we decide that the JavaCall.jl context of the call is more important than the JVM context then we can keep the error string with your checknull macro.
src/core.jl
Outdated
| _jcallable(typ::Type{JavaObject{T}}) where T = metaclass(T) | ||
| function _jcallable(obj::JavaObject) | ||
| isnull(obj) && throw(JavaCallError("Attempt to call method on Java NULL")) | ||
| obj | ||
| end |
There was a problem hiding this comment.
The two forms of this function are quite distinct. The second does some validation. Documenting this would be useful.
For the second form, perhaps the two arg checknull would be useful.
There was a problem hiding this comment.
Maybe we should skip the null-check? I, agree with you on documentation.
When I kept that null-check from before it felt similar to the JVM behavior: A static method is always callable, an instance method is only callable if the instance is non-null. However, that's not really what's going on here. If we just skip the check we get:
Exception in thread "main" java.lang.NullPointerException
ERROR: JavaCall.JavaCallError("Error calling Java: java.lang.NullPointerException")
Stacktrace:
[1] geterror()
@ JavaCall JavaCall.jl/src/core.jl:487
[2] _jcall(::JavaObject [...]
Which could be just as informative.
src/core.jl
Outdated
| # Call instance methods | ||
| function jcall(obj::JavaObject, method::AbstractString, rettype::Type, argtypes::Tuple = (), args... ) | ||
| assertroottask_or_goodenv() && assertloaded() | ||
| function get_method_id(jnifun, ptr, method::AbstractString, rettype::Type, argtypes::Tuple) |
There was a problem hiding this comment.
Do we know that jnifun is a Function and that ptr must be a Ptr? Is there a reason to keep this generic?
There was a problem hiding this comment.
Is an empty tuple still a reasonable default for argtypes?
src/core.jl
Outdated
| for (x, name) in [(:Type, "Object"), | ||
| (:(Type{jboolean}), "Boolean"), | ||
| (:(Type{jchar}), "Char" ), | ||
| (:(Type{jbyte}), "Byte" ), | ||
| (:(Type{jshort}), "Short" ), | ||
| (:(Type{jint}), "Int" ), | ||
| (:(Type{jlong}), "Long" ), | ||
| (:(Type{jfloat}), "Float" ), | ||
| (:(Type{jdouble}), "Double" ), | ||
| (:(Type{jvoid}), "Void" )] |
There was a problem hiding this comment.
Simplify the first component to as before. For just plain type, use :(<:Any). Later you can use rettype::Type{$(x)} as before.
Make the second component, name a Symbol. :Object rather than "Object".
| GC.@preserve savedArgs begin | ||
| result = callmethod(Ptr(obj), jmethodId, Array{JNI.jvalue}(jvalue.(convertedArgs))) | ||
| end | ||
| cleanup_arg.(convertedArgs) |
There was a problem hiding this comment.
A preexisting question is if we should just GC.@preserve the convertedArgs and let the Julia GC and JavaObject finalizer deal with the cleanup. The original issue I suppose is that there currently is no way for the Julia and Java garbage collectors to communicate.
There was a problem hiding this comment.
Leave this alone for now. I'll revisit it later or in another PR.
Let's build some infrastructure here for this around the |
There was a problem hiding this comment.
Here's my suggestion for a @checknull macro and an expanded checknull function.
Here's a demonstration:
julia> @checknull JNI.NewStringUTF(Ptr{UInt8}(C_NULL)) "Issue creating a Java String"
ERROR: JavaCallError("JavaCall.JNI.NewStringUTF: Issue creating a Java String")Later we can write specialized checknull functions:
function checknull(x, msg, y::typeof(JNI.GetMethodID))
...
end
|
Thank you @mkitti for the in-depth review. I may not have time to got through everything today but for the general comments:
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: From a library maintainer/readability perspective, to me, this defines a very clear contract and I would actually prefer removing the
If I want to make a new type (like native arrays) with a The disadvantage, as you indicate, is that we may lose some documentation from a library user's perspective. For example typing
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 |
|
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? |
|
@ahnlabb would you mind I contributed some commits to this branch? |
|
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! |
|
@mkitti did you continue work here (anything not pushed to the branch)? Could I devote some time to this PR the coming weeks? |
|
I have not made progress. Go ahead. |
"No constructor for $T with signature $sig"
|
This is about where I want it now. It could use some more documentation. @aviks , any thoughts about merging this? |
|
@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 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)
endso 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. |
The main purpose of this PR is to make it easier to extend JavaCall with types that e.g. need special cleanup, define a
signaturemethod, etc. (see #144).Since the relevant logic to handle the core call through
JNIwas spread out, discrepancies had crept into the codebase:JavaCall.jl/src/core.jl
Line 485 in 7973dc7
JavaCall.jl/src/core.jl
Line 458 in 7973dc7
So:
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.jlwhere we:GC.@preservefor a Java method call.