improved error handling for read() with concurrent close() (#569)

- isOpen() returns false during concurrent close() - less tracing in SerialInputOutputManager
mik3y · Apr 25, 2024 · 8b9ad7e · 8b9ad7e
1 parent 1245293
commit 8b9ad7e
Show file tree

Hide file tree

Showing 6 changed files with 26 additions and 15 deletions.
diff --git a/usbSerialForAndroid/build.gradle b/usbSerialForAndroid/build.gradle
@@ -13,7 +13,7 @@ android {
 
         testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
         testInstrumentationRunnerArguments = [                    // Raspi   Windows   LinuxVM   ...
-                'rfc2217_server_host': '192.168.0.147',
+                'rfc2217_server_host': '192.168.0.78',
                 'rfc2217_server_nonstandard_baudrates': 'true',   // true    false     false
         ]
     }

diff --git a/usbSerialForAndroid/src/androidTest/java/com/hoho/android/usbserial/DeviceTest.java b/usbSerialForAndroid/src/androidTest/java/com/hoho/android/usbserial/DeviceTest.java
@@ -320,8 +320,10 @@ public void run() {
         try {
             closer.wait = false;
             usb.serialPort.read(new byte[256], 2000);
+
             fail("closed expected");
         } catch(IOException ex) {
+            assertFalse(usb.serialPort.isOpen());
             assertEquals("Connection closed", ex.getMessage());
         }
         th.join();
@@ -333,6 +335,7 @@ public void run() {
             usb.serialPort.read(new byte[256], 0);
             fail("closed expected");
         } catch(IOException ex) {
+            assertFalse(usb.serialPort.isOpen());
             assertEquals("Connection closed", ex.getMessage());
         }
         th.join();

diff --git a/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/CommonUsbSerialPort.java b/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/CommonUsbSerialPort.java
@@ -139,6 +139,8 @@ public void close() throws IOException {
         if (mConnection == null) {
             throw new IOException("Already closed");
         }
+        UsbDeviceConnection connection = mConnection;
+        mConnection = null;
         try {
             mUsbRequest.cancel();
         } catch(Exception ignored) {}
@@ -147,20 +149,22 @@ public void close() throws IOException {
             closeInt();
         } catch(Exception ignored) {}
         try {
-            mConnection.close();
+            connection.close();
         } catch(Exception ignored) {}
-        mConnection = null;
     }
 
     protected abstract void closeInt();
 
     /**
      * use simple USB request supported by all devices to test if connection is still valid
      */
-    protected void testConnection() throws IOException {
-        if(mConnection == null || mUsbRequest == null) {
+    protected void testConnection(boolean full) throws IOException {
+        if(mConnection == null) {
             throw new IOException("Connection closed");
         }
+        if(!full) {
+            return;
+        }
         byte[] buf = new byte[2];
         int len = mConnection.controlTransfer(0x80 /*DEVICE*/, 0 /*GET_STATUS*/, 0, 0, buf, buf.length, 200);
         if(len < 0)
@@ -169,7 +173,7 @@ protected void testConnection() throws IOException {
 
     @Override
     public int read(final byte[] dest, final int timeout) throws IOException {
-        if(dest.length <= 0) {
+        if(dest.length == 0) {
             throw new IllegalArgumentException("Read buffer too small");
         }
         return read(dest, dest.length, timeout);
@@ -179,7 +183,7 @@ public int read(final byte[] dest, final int timeout) throws IOException {
     public int read(final byte[] dest, final int length, final int timeout) throws IOException {return read(dest, length, timeout, true);}
 
     protected int read(final byte[] dest, int length, final int timeout, boolean testConnection) throws IOException {
-        if(mConnection == null || mUsbRequest == null) {
+        if(mConnection == null) {
             throw new IOException("Connection closed");
         }
         if(length <= 0) {
@@ -201,8 +205,8 @@ protected int read(final byte[] dest, int length, final int timeout, boolean tes
             nread = mConnection.bulkTransfer(mReadEndpoint, dest, readMax, timeout);
             // Android error propagation is improvable:
             //  nread == -1 can be: timeout, connection lost, buffer to small, ???
-            if(nread == -1 && testConnection && MonotonicClock.millis() < endTime)
-                testConnection();
+            if(nread == -1 && testConnection)
+                testConnection(MonotonicClock.millis() < endTime);
 
         } else {
             final ByteBuffer buf = ByteBuffer.wrap(dest, 0, length);
@@ -217,7 +221,7 @@ protected int read(final byte[] dest, int length, final int timeout, boolean tes
             // Android error propagation is improvable:
             //   response != null & nread == 0 can be: connection lost, buffer to small, ???
             if(nread == 0) {
-                testConnection();
+                testConnection(true);
             }
         }
         return Math.max(nread, 0);

diff --git a/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/FtdiSerialDriver.java b/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/FtdiSerialDriver.java
@@ -165,8 +165,8 @@ public int read(final byte[] dest, int length, final int timeout) throws IOExcep
                 do {
                     nread = super.read(dest, length, Math.max(1, (int)(endTime - MonotonicClock.millis())), false);
                 } while (nread == READ_HEADER_LENGTH && MonotonicClock.millis() < endTime);
-                if(nread <= 0 && MonotonicClock.millis() < endTime)
-                    testConnection();
+                if(nread <= 0)
+                    testConnection(MonotonicClock.millis() < endTime);
             } else {
                 do {
                     nread = super.read(dest, length, timeout);

diff --git a/...erialForAndroid/src/main/java/com/hoho/android/usbserial/driver/ProlificSerialDriver.java b/...erialForAndroid/src/main/java/com/hoho/android/usbserial/driver/ProlificSerialDriver.java
@@ -205,8 +205,8 @@ private void readStatusThreadFunction() {
                     byte[] buffer = new byte[STATUS_BUFFER_SIZE];
                     long endTime = MonotonicClock.millis() + 500;
                     int readBytesCount = mConnection.bulkTransfer(mInterruptEndpoint, buffer, STATUS_BUFFER_SIZE, 500);
-                    if(readBytesCount == -1 && MonotonicClock.millis() < endTime)
-                        testConnection();
+                    if(readBytesCount == -1)
+                        testConnection(MonotonicClock.millis() < endTime);
                     if (readBytesCount > 0) {
                         if (readBytesCount != STATUS_BUFFER_SIZE) {
                             throw new IOException("Invalid status notification, expected " + STATUS_BUFFER_SIZE + " bytes, got " + readBytesCount);

diff --git a/...ialForAndroid/src/main/java/com/hoho/android/usbserial/util/SerialInputOutputManager.java b/...ialForAndroid/src/main/java/com/hoho/android/usbserial/util/SerialInputOutputManager.java
@@ -203,7 +203,11 @@ public void run() {
                 step();
             }
         } catch (Exception e) {
-            Log.w(TAG, "Run ending due to exception: " + e.getMessage(), e);
+            if(mSerialPort.isOpen()) {
+                Log.w(TAG, "Run ending due to exception: " + e.getMessage(), e);
+            } else {
+                Log.i(TAG, "Socket closed");
+            }
             final Listener listener = getListener();
             if (listener != null) {
               listener.onRunError(e);