Add more error handling in writeBytes. #152
Conversation
hiddenalpha
commented
Nov 9, 2023
hiddenalpha
commented
- Throw NullPointerException (in place of crashing) if caller passes null buffer.
- Pass-through OutOfMemoryError if GetByteArrayElements() fails to alloc memory.
- Throw NullPointerException (in place of crashing) if caller passes null buffer. - Pass-through OutOfMemoryError if GetByteArrayElements() fails to alloc memory. Cherry-picked from 5abdd45.
Can you explain where the
Hmm... Should we wrap exceptions which are historically non-checked? |
return NULL, without any code that would prepare a java exception. In JNI doc I read that an exception gets setup if a JNI function fails. (Read "https://www.developer.com/open-source/exception-handling-in-jni/" to see what I mean) But for whatever reason this seems not to be the case for GetByteArrayElements(). - writeBytes() calls GetByteArrayElements(). - GetByteArrayElements() calls NEW_C_HEAP_ARRAY_RETURN_NULL(). - NEW_C_HEAP_ARRAY_RETURN_NULL() calls NEW_C_HEAP_ARRAY3(). - NEW_C_HEAP_ARRAY3() calls AllocateHeap(). - AllocateHeap() uses malloc() to get memory. Direct Links: GetByteArrayElements: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/prims/jni.cpp#L3597 call to NEW_C_HEAP_ARRAY_RETURN_NULL: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/prims/jni.cpp#L3611 NEW_C_HEAP_ARRAY_RETURN_NULL: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.hpp#L680 call to NEW_C_HEAP_ARRAY3: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.hpp#L681 NEW_C_HEAP_ARRAY3: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.hpp#L668 call to AllocateHeap: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.hpp#L669 AllocateHeap: https://github.com/openjdk/jdk/blob/jdk8-b120/hotspot/src/share/vm/memory/allocation.inline.hpp#L52 Picked from 4a805a7.
|
Will have a look as soon I find some time. Thx for feedback so far. |
Whoops, we don't. We only catch jssc/src/main/java/jssc/SerialPort.java Lines 414 to 416 in ea2ade2 I think unchecked exceptions are fine for now (they're certainly better than a JVM crash). If users complain about this we, can modify the the catch statement at a future time. |
|
@hiddenalpha do you believe we should add more unit tests for these? Furthemore, have you had time to evaluate the safety of the windows code too? |
I think writing unit tests may be challenging for some of those cases. Especially edge-cases in the native code. I think best is to add a test when we see a good opportunity and if it can be written in a simple (as in understandable/maintainable) way.
Did not look at the windows code at all yet. Will keep it in mind when I've a free time slot. |
|
This can be tested in its current form using It looks good to me, but I've left a question about the unit test, since matching an exception string can have unobvious test failures in the event this string value ever gets changed. |
…ng-in-writeBytes-20231109 Conflicts: src/test/java/jssc/SerialNativeInterfaceTest.java
|
When you have time, this PR needs to be rebased and there's still one comment remaining. I've love to see it merged. :) |
Taken from 618158e .
This adds the risk of false results as the test now will pass for ANY kind of NPE. But who cares.
