java bindings error handling could use improving

[hppritcha]

The OMPI native method error handling code ompi_java_exceptionCheck could use some improving. In particular, there are some cases where the MPI native code might return MPI_SUCCESS, but actually the JNI support code detected something was amiss and had thrown an exception. Currently this case is not being handled correctly.

The two attached test cases illustrate the problem. In the first case, the MPI native code returns an error, which ompi_jav_exceptionCheck correctly handles. The second case, an out-of-bounds error from

java.lang.ArrayIndexOutOfBoundsException

is not handled correctly.

Thanks to Siegmar for reporting ( http://www.open-mpi.org/community/lists/users/2016/05/29256.php)
Exception_1_Main.txt
Exception_2_Main.txt

[ggouaillardet]

@nrgraham23 can you please comment on Siegmar's claim (and my replies) that was initially posted at https://mail-archive.com/users@lists.open-mpi.org/msg29868.html ?

[nrgraham23]

@ggouaillardet @siegmargross @hppritcha

Typically in Java land if something goes wrong in user code, an exception is thrown back to the user for them to handle. So in this case, an IndexOutOfBoundsException occurs because the user is attempting to access an element of the array that is not there.

When I made this fix, I fixed a logic error in the code, however for the scenario Siegmar provided, the logic change would have no effect, because I still let the error be thrown back to the user. I think this is the way it should be, and Java has tools for handling this case. I have attached a file with a modified version of the test that shows how I think this kind of thing should be handled (change .txt to .java of course).

Exception2Main.txt

[nrgraham23]

I realized I should have said much of what I said in a reply to Gilles from a comment on the commit where I merged the code to fix this issue, actually here in the issue itself. So Ill copy paste it below.

Gilles wrote:

@nrgraham23 the result of this function is not checked in collectives. shouldn't we "cancel" the native exception and throw a MPIException here ?
my knowledge of C/Java interaction is pretty limited, so the terminology I used might be incorrect.

I replied:

@ggouaillardet @hppritcha @jsquyres @siegmargross

I would say no. JNI exceptions are directly related to problems with something the JNI has tried to do. It seems to me like this is a separate issue from MPI because it is a result of a user trying to do something they shouldn't in Java. In the case of the index out of bounds error that has brought this up, a java array is too short for the attempted call.

That said, I have thoroughly traced the way the exceptions are being thrown, and discovered that it is actually acting differently from the way I had originally thought. The ompi_java_exceptionCheck method is returning a true when a JNI error occurs, however it's return is almost never checked (you can do a "git grep ompi_java_exceptionCheck" from the top ompi directory to see what I am talking about).

Instead, in almost every case, the code continues to execute in the C code, and the Java exception is not actually thrown until it returns to Java land.

I propose we change it to either throw an exception in ompi_java_exceptionCheck back to Java so the code stops executing and change the method return to void, or change all of the places where ompi_java_exceptionCheck is being called to actually check the return value.

The first option is far less code, but the second option would allow us to do memory cleanup stuff. Any thoughts?

[hppritcha]

I think the second approach is better - assuming sufficient time to implement.

[nrgraham23]

Upon further reading I have learned that throwing your own exception in the JNI will not return the code immediately to Java land, the C code always finishes running and exceptions are thrown when it returns to Java.

With this is mind, I think the best course of action will be to check the return of ompi_java_exceptionCheck. If it returns true, do any cleanup and return out of the C code.

Ill work on changing the bindings to check the result of ompi_java_exceptionCheck, but there is a fair amount to do and it will take some time.

@siegmargross Thanks for bringing this back up. There is some work to be done here yet, but we will still throw Java related exceptions back to the user. The version of Exception2Main I provided above is how such errors should be handled.