Report some read errors back to java code. (#155)

Report some read errors back to java code.
* Relates to #126
* Taken from 19610bc
* Taken from 3dcb4ec

src/main/cpp/_nix_based/jssc.cpp

@@ -549,22 +549,51 @@ JNIEXPORT jboolean JNICALL Java_jssc_SerialNativeInterface_writeBytes
 
 /**
  * Waits until 'read()' has something to tell for the specified filedescriptor.
+ *
+ * Returns zero on success. Returns negative values on error and may
+ * sets up a java exception in 'env'.
  */
-static void awaitReadReady(JNIEnv*, jlong fd){
+static int awaitReadReady(JNIEnv*env, jlong fd){
+    int err;
+    int numUnknownErrors = 0;
 #if HAVE_POLL == 0
     // Alternative impl using 'select' as 'poll' isn't available (or broken).
 
-    //assert(fd < FD_SETSIZE); // <- Might help when hunting SEGFAULTs.
+    if( fd >= FD_SETSIZE ){
+        jclass exClz = env->FindClass("java/lang/UnsupportedOperationException");
+        if( exClz != NULL ) env->ThrowNew(exClz, "Bad luck. 'select' cannot handle large fds.");
+        static_assert(EBADF > 0, "EBADF > 0");
+        return -EBADF;
+    }
     fd_set readFds;
     while(true) {
         FD_ZERO(&readFds);
         FD_SET(fd, &readFds);
         int result = select(fd + 1, &readFds, NULL, NULL, NULL);
-        if(result < 0){
-            // man select: On error, -1 is returned, and errno is set to indicate the error
-            // TODO: Maybe a candidate to raise a java exception. But won't do
-            //       yet for backward compatibility.
-            continue;
+        if( result < 0 ){
+            err = errno;
+            jclass exClz = NULL;
+            switch( err ){
+                case EBADF:
+                    exClz = env->FindClass("java/lang/IllegalArgumentException");
+                    if( exClz != NULL ) env->ThrowNew(exClz, "EBADF select()");
+                    static_assert(EBADF > 0, "EBADF > 0");
+                    return -err;
+                case EINVAL:
+                    exClz = env->FindClass("java/lang/IllegalArgumentException");
+                    if( exClz != NULL ) env->ThrowNew(exClz, "EINVAL select()");
+                    static_assert(EINVAL > 0, "EINVAL > 0");
+                    return -err;
+                default:
+                    // TODO: Maybe other errors are candidates to raise java exceptions too. We can
+                    //       add them as soon we know which of them occur, and what they actually
+                    //       mean in our context. For now, we infinitely loop, as the original code
+                    //       did.
+                    if( numUnknownErrors == 0) fprintf(stderr, "select(): %s\n", strerror(err));
+                    static_assert(INT_MAX >= 0x7FFF, "INT_MAX >= 0x7FFF");
+                    numUnknownErrors = (numUnknownErrors + 1) & 0x3FFF;
+                    continue;
+            }
         }
         // Did wait successfully.
         break;
@@ -580,17 +609,32 @@ static void awaitReadReady(JNIEnv*, jlong fd){
     fds[0].events = POLLIN;
     while(true){
         int result = poll(fds, 1, -1);
-        if(result < 0){
-            // man poll: On error, -1 is returned, and errno is set to indicate the error.
-            // TODO: Maybe a candidate to raise a java exception. But won't do
-            //       yet for backward compatibility.
-            continue;
+        if( result < 0 ){
+            err = errno;
+            jclass exClz = NULL;
+            switch( err ){
+                case EINVAL:
+                    exClz = env->FindClass("java/lang/IllegalArgumentException");
+                    if( exClz != NULL ) env->ThrowNew(exClz, "EINVAL poll()");
+                    static_assert(EINVAL > 0, "EINVAL > 0");
+                    return -err;
+                default:
+                    // TODO: Maybe other errors are candidates to raise java exceptions too. We can
+                    //       add them as soon we know which of them occur, and what they actually
+                    //       mean in our context. For now, we infinitely loop, as the original code
+                    //       did.
+                    if( numUnknownErrors == 0) fprintf(stderr, "poll(): %s\n", strerror(err));
+                    static_assert(INT_MAX >= 0x7FFF, "INT_MAX >= 0x7FFF");
+                    numUnknownErrors = (numUnknownErrors + 1) & 0x3FFF;
+                    continue;
+            }
         }
         // Did wait successfully.
         break;
     }
 
 #endif
+    return 0;
 }
 
 /* OK */
@@ -602,23 +646,40 @@ static void awaitReadReady(JNIEnv*, jlong fd){
 JNIEXPORT jbyteArray JNICALL Java_jssc_SerialNativeInterface_readBytes
   (JNIEnv *env, jobject, jlong portHandle, jint byteCount){
 
-    // TODO: Errors should be communicated by raising java exceptions; Will break
-    //       backwards compatibility.
-
+    int err;
     jbyte *lpBuffer = new jbyte[byteCount];
     jbyteArray returnArray = NULL;
     int byteRemains = byteCount;
 
     while(byteRemains > 0) {
         int result = 0;
 
-        awaitReadReady(env, portHandle);
+        err = awaitReadReady(env, portHandle);
+        if( err < 0 ){
+            /* nothing we could read. */
+            if( byteRemains != byteCount ){
+                /* return what we already have so far. */
+                env->ExceptionClear();
+                break;
+            }else{
+                /* nothing we could return. pass-through exception */
+                assert(env->ExceptionCheck());
+                returnArray = NULL; goto Finally;
+            }
+        }
 
         errno = 0;
         result = read(portHandle, lpBuffer + (byteCount - byteRemains), byteRemains);
         if (result < 0) {
-            // man read: On error, -1 is returned, and errno is set to indicate the error.
-            // TODO: May candidate for raising a java exception. See comment at begin of function.
+            err = errno;
+            const char *exName = NULL, *emsg = NULL;
+            switch( err ){
+                case EBADF: exName = "java/lang/IllegalArgumentException"; emsg = "EBADF"; break;
+                default: exName = "java/io/IOException"; emsg = strerror(err); break;
+            }
+            jclass exClz = env->FindClass(exName);
+            if( exClz != NULL ) env->ThrowNew(exClz, emsg);
+            returnArray = NULL; goto Finally;
         }
         else if (result == 0) {
             // AFAIK this happens either on EOF or on EWOULDBLOCK (see 'man read').
@@ -630,8 +691,12 @@ JNIEXPORT jbyteArray JNICALL Java_jssc_SerialNativeInterface_readBytes
         }
     }
 
-    returnArray = env->NewByteArray(byteCount);
-    env->SetByteArrayRegion(returnArray, 0, byteCount, lpBuffer);
+    returnArray = env->NewByteArray(byteCount - byteRemains);
+    if( returnArray == NULL ) goto Finally;
+    env->SetByteArrayRegion(returnArray, 0, byteCount - byteRemains, lpBuffer);
+    assert(env->ExceptionCheck() == JNI_FALSE);
+
+Finally:
     delete[] lpBuffer;
     return returnArray;
 }

src/main/cpp/windows/jssc.cpp

@@ -266,8 +266,8 @@ JNIEXPORT jbyteArray JNICALL Java_jssc_SerialNativeInterface_readBytes
     HANDLE hComm = (HANDLE)portHandle;
     DWORD lpNumberOfBytesTransferred;
     DWORD lpNumberOfBytesRead;
-    OVERLAPPED *overlapped = new OVERLAPPED();
     jbyte *lpBuffer = NULL;
+
     jbyteArray returnArray = env->NewByteArray(byteCount);
 
     lpBuffer = (jbyte *)malloc(byteCount * sizeof(jbyte));
@@ -276,6 +276,7 @@ JNIEXPORT jbyteArray JNICALL Java_jssc_SerialNativeInterface_readBytes
         return returnArray;
     }
 
+    OVERLAPPED *overlapped = new OVERLAPPED();
     overlapped->hEvent = CreateEventA(NULL, true, false, NULL);
     if(ReadFile(hComm, lpBuffer, (DWORD)byteCount, &lpNumberOfBytesRead, overlapped)){
         env->SetByteArrayRegion(returnArray, 0, byteCount, lpBuffer);
@@ -287,6 +288,10 @@ JNIEXPORT jbyteArray JNICALL Java_jssc_SerialNativeInterface_readBytes
             }
         }
     }
+    else if(GetLastError() == ERROR_INVALID_HANDLE){
+        jclass exClz = env->FindClass("java/lang/IllegalArgumentException");
+        if( exClz != NULL ) env->ThrowNew(exClz, "EBADF");
+    }
     CloseHandle(overlapped->hEvent);
     delete overlapped;
     free(lpBuffer);

src/main/java/jssc/SerialNativeInterface.java

@@ -267,7 +267,7 @@ public static String getLibraryVersion() {
      * 
      * @return Method returns the array of read bytes
      */
-    public native byte[] readBytes(long handle, int byteCount);
+    public native byte[] readBytes(long handle, int byteCount) throws IOException;
 
     /**
      * Write data to port

src/main/java/jssc/SerialPort.java

@@ -512,7 +512,11 @@ public boolean writeIntArray(int[] buffer) throws SerialPortException {
      */
     public byte[] readBytes(int byteCount) throws SerialPortException {
         checkPortOpened("readBytes()");
-        return serialInterface.readBytes(portHandle, byteCount);
+        try{
+            return serialInterface.readBytes(portHandle, byteCount);
+        }catch( IOException ex ){
+            throw SerialPortException.wrapNativeException(ex, this, "readBytes");
+        }
     }
 
     /**

src/test/java/jssc/SerialNativeInterfaceTest.java

@@ -1,17 +1,20 @@
 package jssc;
 
 import org.junit.Assume;
+import org.junit.Before;
 import org.junit.Test;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.CoreMatchers.nullValue;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeFalse;
+import static org.junit.Assume.assumeTrue;
 
 public class SerialNativeInterfaceTest {
 
-
     @Test
     public void testInitNativeInterface() {
         SerialNativeInterface serial = new SerialNativeInterface();
@@ -50,4 +53,39 @@ public void reportsWriteErrorsAsIOException() throws Exception {
         testTarget.writeBytes(fd, buf);
     }
 
+    @Test
+    public void throwsIllegalArgumentExceptionIfPortHandleIllegal() throws Exception {
+        assumeFalse(SerialNativeInterface.getOsType() == SerialNativeInterface.OS_MAC_OS_X);
+
+        SerialNativeInterface testTarget = new SerialNativeInterface();
+        try{
+            testTarget.readBytes(999, 42);
+            fail("Where is the exception?");
+        }catch( IllegalArgumentException ex ){
+            assertTrue(ex.getMessage().contains("EBADF"));
+        }
+    }
+
+    /**
+     * <p>This is a duplicate of {@link #throwsIllegalArgumentExceptionIfPortHandleIllegal()}
+     * but targets osx only. Not yet analyzed why osx (using select) hangs here. See also <a
+     * href="https://github.com/java-native/jssc/pull/155">PR 155</a>. Where this
+     * was discovered.</p>
+     *
+     * <p>TODO: Go down that rabbit hole and get rid of that "bug".</p>
+     */
+    @Test
+    @org.junit.Ignore("TODO analyze where this osx hang comes from")
+    public void throwsIllegalArgumentExceptionIfPortHandleIllegalOsx() throws Exception {
+        assumeTrue(SerialNativeInterface.getOsType() == SerialNativeInterface.OS_MAC_OS_X);
+
+        SerialNativeInterface testTarget = new SerialNativeInterface();
+        try{
+            testTarget.readBytes(999, 42);
+            fail("Where is the exception?");
+        }catch( IllegalArgumentException ex ){
+            assertTrue(ex.getMessage().contains("EBADF"));
+        }
+    }
+
 }