search

java-native / jssc

Java library for talking to serial ports (with added build support for maven, cmake, MSVC)
https://discord.gg/RBsUfE9sX9
GNU Lesser General Public License v3.0
170 stars 53 forks source link

Add more error handling in writeBytes. #152

Closed hiddenalpha closed 11 months ago

hiddenalpha commented 12 months ago
tresf commented 12 months ago

Pass-through OutOfMemoryError if GetByteArrayElements() fails to alloc memory.

Can you explain where the OutOfMemoryError is thrown?

Throw NullPointerException (in place of crashing) if caller passes null buffer.

Hmm... Should we wrap exceptions which are historically non-checked?

hiddenalpha commented 12 months ago

Will have a look as soon I find some time. Thx for feedback so far.

tresf commented 12 months ago

Hmm... Should we wrap exceptions which are historically non-checked?

Will have a look as soon I find some time. Thx for feedback so far.

Whoops, we don't. We only catch IOException, so we're still going to throw unchecked exceptions, which I think is what we want, per:

https://github.com/java-native/jssc/blob/ea2ade2b3cbbc417ef9dfd97922a77fde41b911a/src/main/java/jssc/SerialPort.java#L414-L416

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.

tresf commented 12 months ago

@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?

hiddenalpha commented 12 months ago

do you believe we should add more unit tests for these?

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.

have you had time to evaluate the safety of the windows code too?

Did not look at the windows code at all yet. Will keep it in mind when I've a free time slot.

tresf commented 11 months ago

This can be tested in its current form using mvn -DskipTests install && mvn until rebased against master (specifically #51, #151).

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.

tresf commented 11 months ago

When you have time, this PR needs to be rebased and there's still one comment remaining. I've love to see it merged. :)