#72 java: idiomatic exceptions

[phraktle]

I did a quick pass on making the Java API a bit more idiomatic regarding thrown exceptions. In this patch, the appropriate runtime exceptions are thrown when checking arguments / state. Some method signatures were fixed (where possible without touching native code).

Ideally, there should be another round of review to make the native parts throw more appropriate exceptions (subclasses of RuntimeException where appropriate, or some subclass of IOException for cases where you might need checked exceptions).

[DRC]

This looks good to me, but I agree that it would make the most sense to also use idiomatic exceptions at the JNI level. So, some questions:

-- Given that the only sane way to handle errors from the native library is to pass those through using a checked exception, is there any advantage to using IOExceptions in that case as opposed to plain Exceptions?

-- There are definitely some instances in the JNI code in which I can use IllegalArgumentException, but how do I handle the case of NULL being returned from one of the JNI methods? This seems like it should be an unchecked exception, but NullPointerException doesn't seem like the correct idiom.

[DRC]

I should clarify what I mean in the second question. If, for instance, I call

cBuffer=(*env)->GetPrimitiveArrayCritical(env, javaBuffer, 0)

within the JNI code, I want to throw an exception if cBuffer happens to be NULL for some reason, because that will allow the Java program to abort gracefully instead of segfaulting the JVM.

[DRC]

Never mind. Actually, in all cases, the underlying JNI methods (GetFieldID(), etc.) will throw an exception whenever they return NULL, so the custom exception within the bailif0() macro would never have been thrown anyhow. In those cases, I now just detect the error and abort the C function immediately. The JNI code now throws IllegalArgumentException for argument errors and OutOfMemoryError for malloc() errors. It still uses java.lang.Exception for errors passed through from the C API, so let me know if it makes sense to use IOException for those.

Code has been checked into trunk.

[phraktle]

Hi DRC,

Sounds good!

Using the top-level Exception is certainly too generic, since that forces the caller to indiscriminately catch all exceptions (checked & unchecked) when calling the API. So at the very least, I would recommend using custom exceptions and changing the signatures in the API to throw those, rather than Exception.

Whether you use a checked or unchecked exception for signaling error conditions is somewhat a matter of taste. To indicate programming errors, I would always go with unchecked, such as IllegalState, etc. Simple things like a constructor or getter/setter should not throw checked exceptions either.

I would go with checked exceptions only in cases where the caller can be expected to have a reasonable way to recover locally (e.g. "I can't decode the image, so let me use a fallback").

Long story short:

• never use "throws Exception" (too generic)

• the following should definitely not throw a checked exception (since there's no sane way for the caller to recover anyway):

• the native inits/destroys

• simple setters/getters (e.g. TJ#getScalingFactors, YUVImage#planeXXX, etc)

• The decompress/compress calls may be justified in using a checked exception, to indicate invalid image data or such (which would be a subclass of IOException).

(My personal preference would be an unchecked exception even in these cases, i.e. JpegDecodeException extends RuntimeException. This way callers can handle this if they want to, otherwise let it propagate transparently. This works better with Java8 lambdas too, which cannot throw checked exceptions).

Regards,
Viktor

[phraktle]

I will submit a further patch shortly with a reasonable exception signature...