Make JSSC exceptions extend IOException instead of Exception

[chabala]

Make JSSC exceptions extend IOException instead of Exception.

The change

This is a change in the class hierarchy for both custom exceptions in JSSC. Both currently extend Exception, and this changes them to extend IOException instead, which is a subclass of Exception. This naturally makes sense as any serial port interactions are IO interactions. This should be a transparent change to all existing code, as any code catching the exceptions by exact name would be unaffected, and any code catching Exception will also catch IOException without change.

The why

If I'm writing a library that interacts with the serial interface, almost every method throws exceptions, and most of the time I want to pass them on to my caller to decide how to handle them. So if my code has a public method that interacts with the serial interface, I can:

1. Add throws jssc.SerialPortException to my method signature (and potentially also jssc.SerialPortTimeoutException, since they don't have a common parent other than Exception)
2. Add throws Exception to my method signature
3. Catch jssc.SerialPortException myself and wrap in a more neutral exception, like IOException.

Option one is gross because it leaks JSSC classes in my public API signatures to my clients. As a direct user of JSSC, I don't care much about what exceptions JSSC defines. But as a library author that uses JSSC, I don't want JSSC classes to leak into my public API, as the underlying serial library is an implementation detail. This would limit me if I wanted to support adapters for other serial libraries.

Option two is gross because anything that causes one to need to catch Exception is very bad form, because then there's the risk of catching exceptions you didn't want to catch like OutOfMemoryError.

That leaves me with option three. In every JSSC serial project I make, there ends up being an adapter to wrap the serial port and sanitize the exceptions, a la:

    /** {@inheritDoc} */
    @Override
    public boolean write(byte b) throws IOException {
        try {
            return serialPort.writeByte(b);
        } catch (SerialPortException e) {
            throw new IOException(e);
        }
    }

    /** {@inheritDoc} */
    @Override
    public boolean write(byte[] bytes) throws IOException {
        try {
            return serialPort.writeBytes(bytes);
        } catch (SerialPortException e) {
            throw new IOException(e);
        }
    }

    /** {@inheritDoc} */
    @Override
    public byte[] readBytes(int byteCount) throws IOException {
        try {
            return serialPort.readBytes(byteCount);
        } catch (SerialPortException e) {
            throw new IOException(e);
        }
    }

    /** {@inheritDoc} */
    @Override
    public void close() throws IOException {
        if (serialPort.isOpened()) {
            try {
                serialPort.closePort();
            } catch (SerialPortException e) {
                throw new IOException("Error closing serial port", e);
            }
        }
    }

Hence this change, which by moving the exceptions to extend IOException make all of this wrapping unnecessary, as the client can catch IOException without knowing the JSSC specific exceptions exist.

[tresf]

This should be a transparent change to all existing code

Agreed.

This naturally makes sense as any serial port interactions are IO interactions.

This statement is leading. Although a serial port interaction is an IO interaction, JSSC uses exceptions in a rather unorthodox method by using a form of catch-all serialization. For example, the following exceptions are arguably NOT IOExceptions, but rather API exceptions/guards.

    final public static String TYPE_LISTENER_ALREADY_ADDED = "Event listener already added";
// ...
    final public static String TYPE_NULL_NOT_PERMITTED = "Null not permitted";

Perhaps the outliers should be moved to more intuitive parent classes (e.g. TYPE_NULL_NOT_PERMITTED ~= NullPointerException) but this a non-checked exception so I think that would be a regression.

In regards to the downstream usefulness, I'm personally not very sympathetic since so many native libraries have this same exception-pollution-effect.

Example:

• HidException extends RuntimeException (hid4java)
• UsbException extends Exception (usb4java via JSR80)
• Win32Exception extends RuntimeException (jna)
• SerialPortException extends Exception (jssc)

... I have a project which uses all of these requiring the same "gross" wrappers as described. 😏

... anyway, this is just food for thought. In regards to the PR, I have no objections to the proposed change. I'll see how @pietrygamat feels.

[chabala]

For example, the following exceptions are arguably NOT IOExceptions, but rather API exceptions/guards.

I certainly see your point here, though if I'm considering my interface with JSSC as the boundary, I don't feel like the distinction is useful. If the serial library throws an exception, I've either got an issue beyond my software's control, or in the cases highlighted, I've made a programming error. As you point out, any more drastic changes to the exception hierarchy start to break the backward compatibility with @scream3r's JSSC, which is a useful feature.

• HidException extends RuntimeException (hid4java)
• UsbException extends Exception (usb4java via JSR80)
• Win32Exception extends RuntimeException (jna)
• SerialPortException extends Exception (jssc)

Nice overview 👍
I'll also note:

• Sun's JavaComm has three exceptions that extend Exception 😞, but also throws IOException directly
• jSerialComm has different exceptions that extend IOException & RuntimeException 👍
• SerialPundit has all exceptions extending IOException
• RxTx mimics JavaComm by shadowing JavaComm exceptions and using IOException directly.

[tresf]

I don't feel like the distinction is useful.

I started typing a snarky response, but seeing as I actually use JSSC in a similar fashion (blindly raise the exception to user space when they occur) it would be hypocritical of me to continue pandering as the devil's advocate. 😈 🤣

[pietrygamat]

The idea of this change strikes me as something desirable. Ease of use may outweight the religious adherence to the strict definitions of what is and what is not an IOException.
The case of

final public static String TYPE_NULL_NOT_PERMITTED = "Null not permitted";

It would perhaps make sense to throw raw NullPointerException directly in:

public SerialPort(String portName) {
        this.portName = portName;
        serialInterface = new SerialNativeInterface();
    }

when user is actually trying to illegaly assign null to final field, and not wait with raising this issue later in openPort. This would only break BAD user code and only at runtime.

However, the exceptions around EventListeners are indeed something else, and these will be rather caused by preventable user code errors. Naming these IOExceptions would perhaps be misleading. Additing a separate Exception family for those may be a way to go, but it changes the scope of this PR significantly.

Also, the change is NOT entirely transparent to the existing code base. See for example:

        try {
            SerialPort port = new SerialPort("/dev/ttyUSB0");
            port.openPort();
            new FileOutputStream("/tmp/data").write(port.readBytes());
        } catch (IOException ex) {
            System.out.println("Problem with your file system");
        } catch (SerialPortException ex) {
            System.out.println("Problem with your serial device");
        }

That little code snipped is now causing error. The assumption that IOErrors are usually on the part of the file system, but never on the part of the serial device may be shaping the existing catch blocks and recovery strategies?

If we accept this PR - we should perhaps include migration guide in the release notes, and bump version (e.g. 2.10.0 vs 2.9.6), right?

[tresf]

Also, the change is NOT entirely transparent to the existing code base. See for example: (snip)

Fantastic point.

If we accept this PR - we should perhaps include migration guide in the release notes, and bump version (e.g. 2.10.0 vs 2.9.6), right?

Sound like a great compromise.

[chabala]

It would perhaps make sense to throw raw NullPointerException directly

I could add this change to the PR if you prefer. I agree it would only break very bizarre user code.

Would you keep the then unused String constant or strike it as well? I noticed TYPE_PARAMETER_IS_NOT_CORRECT is hanging around unreferenced. I could remove that as well.

the exceptions around EventListeners are indeed something else

I'd prefer the listener registration/deregistration be idempotent or tolerant of multiple listeners rather than throw any exceptions, but I'd advocate for leaving changes around that for another PR.

the change is NOT entirely transparent to the existing code base. See for example:

I agree, this had not occurred to me. Unfortunately I don't think there's a way to know how prevalent this might be in user code.

we should perhaps include migration guide in the release notes

Is there a migration guide in the code tree somewhere? I didn't see any Maven site documents.

[tresf]

No need to modify the PR. We'd likely add it to the GitHub release notes.

[pietrygamat]

I could add this change to the PR if you prefer.
TYPE_PARAMETER_IS_NOT_CORRECT is hanging around unreferenced. I could remove that as well.

Please include the changes in the PR. Because the scope of this PR comes with some uncertainty, I will go ahead with #162 as is, so that we have flexibility to include more drastic changes later, and users have a version to fallback to. We can then safely (I think?) merge this one and continue discussion on potentially changing the other exception behavior.