SerialPort.closePort not calling native interface on Windows when USB serial adapter is disconnected

[pietrygamat]

As noted in #63, There are several methods in the library that are described in javadoc as returning true or false to signify a success, but in fact throwing an exception on failure. The particulary nasty stack for us is:
closePort -> removeEventListener -> setEventMask because in the case where USB serial adapter is already disconnected, this throws an exception before closePort -> serialInterface.closePort(portHandle); is fired.

As a result, the device is not released. Some other software we use that also queries serial ports still sees that ghost port (or sees multiple COM ports, with duplicated number if the adapter is reconnected), even when Device Manager does not.

[tresf]

Perhaps this code:

Edit: Hmm... the editor says that swallows the exception, so, nope. :)

Show code

     public synchronized boolean closePort() throws SerialPortException {
-        checkPortOpened("closePort()");
-        if(eventListenerAdded){
-            removeEventListener();
-        }
-        boolean returnValue = serialInterface.closePort(portHandle);
-        if(returnValue){
-            maskAssigned = false;
-            portOpened = false;
+        boolean returnValue = false;
+        try {
+            checkPortOpened("closePort()");
+            if (eventListenerAdded) {
+                removeEventListener();
+            }
+        } finally{
+            returnValue = serialInterface.closePort(portHandle);
+            if (returnValue) {
+                maskAssigned = false;
+                portOpened = false;
+            }
         }
         return returnValue;
     }

[tresf]

another idea, guard checkPortOpened() and only call once.

Click to see diff

--- a/src/main/java/jssc/SerialPort.java
+++ b/src/main/java/jssc/SerialPort.java
@@ -1049,8 +1049,17 @@ public class SerialPort {
      * 
      * @throws SerialPortException
      */
-    public synchronized boolean removeEventListener() throws SerialPortException {
-        checkPortOpened("removeEventListener()");
+    public synchronized boolean removeEventListener(boolean force) throws SerialPortException {
+        try {
+            checkPortOpened("removeEventListener()");
+        } catch(SerialPortException e) {
+            if(!force) {
+                throw e;
+            } else {
+                // Swallow exception, but put it to the terminal
+                e.printStackTrace();
+            }
+        }
         if(!eventListenerAdded){
             throw new SerialPortException(this, "removeEventListener()", SerialPortException.TYPE_CANT_REMOVE_LISTENER);
         }
@@ -1071,6 +1080,10 @@ public class SerialPort {
         return true;
     }
 
+    public synchronized boolean removeEventListener() throws SerialPortException {
+        return removeEventListener(false);
+    }
+
     /**
      * Close port. This method deletes event listener first, then closes the port
      *
@@ -1079,9 +1092,9 @@ public class SerialPort {
      * @throws SerialPortException
      */
     public synchronized boolean closePort() throws SerialPortException {
-        checkPortOpened("closePort()");
+        // checkPortOpened("closePort()"); // can this cause native closePort() to segfault?
         if(eventListenerAdded){
-            removeEventListener();
+            removeEventListener(true);
         }
         boolean returnValue = serialInterface.closePort(portHandle);
         if(returnValue){

[pietrygamat]

FYI, our production workaround is this, but this particular app is not very portable, so not much testing is done outside of Windows, and perhaps adhering to own javadoc and NOT throwing these exceptions in the first place is the way to go?

public class SerialPortEnhanced extends SerialPort {
    SerialPortEnhanced(String portName) {
        super(portName);
    }

    @Override
    public boolean setEventsMask(int mask) {
        try {
            return super.setEventsMask(mask);
        } catch (SerialPortException e) {
            return false;
        }
    }
}

[tresf]

I think the proper solution will:

• remove listeners regardless if the port is opened
• attempt to close the port even if it is already closed

I do not see many use-cases for throwing an exception before cleanup, except perhaps the Thread.join call, but even then, I would expect eventThread.terminateThread() to be called AFTER Thread.join (not before) if currentThread().getId() differs.

[tresf]

adhering to own javadoc and NOT throwing these exceptions in the first place is the way to go?

I do not see many use-cases for throwing an exception

🤣

[Vzor-]

I think a more thorough examination of what methods should error vs return false should be done next.

[tresf]

would expect eventThread.terminateThread() to be called AFTER Thread.join

Correction... This is wrong; join() is a blocking operation, we must join after we tell the thread to shutdown.

[tresf]

We have what I believe to be a working patch available in #181. After re-reading #123 I believe we have another use-case to guard for. I've pinged @Vzor- in that thread as well. Naturally, any testing on this would be greatly appreciated.

There are several methods in the library that are described in javadoc as returning true or false to signify a success, but in fact throwing an exception on failure.

This is certainly the case for removeEventListener, which I believe should only throw an exception when the port has vanished. This means that it will return true if successful, false if no listener was added and an exception in the rare event that the port disappeared or closed unexpectedly.

@Vzor- wanted to write some unit tests for these scenarios, but neither him nor I knew the best way to do so using our test libraries. If tests cannot be automated, we may choose to create a QA checklist for manually tested items to perform each release to try to catch regressions.

@Vzor- and I discussed the behavior of closePort() as well, and we believe that it shouldn't throw an exception in most scenarios either. To that point, @Vzor-, returnValue = serialInterface.closePort(portHandle); seems to be missing a guard now in the event the port was never opened or has since closed. This is a direct call to native, so I'm not sure what the behavior would be for a port handle that has gone away. Optimistically, this just returns false and we use the API as described, worst-case, it causes a crash. This should be pretty easy to test manually as well.

Since these fixes change the behavior of the API, we may decide to bump the version number to indicate this. Feedback welcome.

[Vzor-]

@Vzor- and I discussed the behavior of closePort() as well, and we believe that it shouldn't throw an exception in most scenarios either. To that point, @Vzor-, returnValue = serialInterface.closePort(portHandle); seems to be missing a guard now in the event the port was never opened or has since closed. This is a direct call to native, so I'm not sure what the behavior would be for a port handle that has gone away. Optimistically, this just returns false and we use the API as described, worst-case, it causes a crash. This should be pretty easy to test manually as well.

My testing shows this works without crashing on linux and windows, though as you mentioned it is relying on native code. The relevant code is here for windows, and here for nix. I believe it is better to rely on the native layer code for the redundant close, but I would feel much better if we had some tests to verify we never hit a native layer crash. I will look into the jni calls a bit to make sure there are no obvious traps.

[Vzor-]

I was never able to get a crash, but I could not think of a benefit of not guarding closePort against duplicate close calls. I originally imagined a situation where closePort silently failed, and led to a port being left open even though the lib thinks it is closed. In this case, there would be some value in trying a close again, but this section of code

returnValue = serialInterface.closePort(portHandle);
if (returnValue) {
    maskAssigned = false;
    portOpened = false;
}

only sets the port to closed, if the return value was true. I think we can reasonably assume it is very unlikely that portOpened could erroneously be set to false.

[tresf]

I was never able to get a crash, but I could not think of a benefit of not guarding closePort against duplicate close calls. I originally imagined a situation where closePort silently failed, and led to a port being left open even though the lib thinks it is closed. In this case, there would be some value in trying a close again, but this section of code

returnValue = serialInterface.closePort(portHandle);
if (returnValue) {
maskAssigned = false;
portOpened = false;
}
only sets the port to closed, if the return value was true. I think we can reasonably assume it is very unlikely that portOpened could erroneously be set to false.

It may be safer to move this guard to the one native call -- setEventsMask(0); -- in removeEventListener so that if a situation occurs where a port is closed but a listener still exists, we don't have a lingering listener attached. I understand that the code flow doesn't allow this currently, but in a future situation where we use a trigger from native to set portOpened = false from native, this could come back to haunt us.

[tresf]

diff --git a/src/main/java/jssc/SerialPort.java b/src/main/java/jssc/SerialPort.java
index 2c8bb1d..1022813 100644
--- a/src/main/java/jssc/SerialPort.java
+++ b/src/main/java/jssc/SerialPort.java
@@ -1157,7 +1157,9 @@ public class SerialPort {
             return false;
         }
         eventThread.terminateThread();
-        setEventsMask(0);
+        if (!portOpened) {
+            setEventsMask(0);
+        }
         //Guard against currentThread().join deadlock
         if(Thread.currentThread().getId() != eventThread.getId()){
             try {
@@ -1182,17 +1184,20 @@ public class SerialPort {
      * @throws SerialPortException if exception occurred
      */
     public synchronized boolean closePort() throws SerialPortException {
-        if (!portOpened) return false;
         boolean returnValue;
         //removeEventListener calls setEventsMask, and must occur before calling closePort
         try {
             removeEventListener();
         }
         finally {
-            returnValue = serialInterface.closePort(portHandle);
-            if (returnValue) {
-                maskAssigned = false;
-                portOpened = false;
+            if(portOpened) {
+                returnValue = serialInterface.closePort(portHandle);
+                if (returnValue) {
+                    maskAssigned = false;
+                    portOpened = false;
+                }
+            } else {
+                returnValue = false;
             }
         }
         return returnValue;