Result -1 of Select() when reading from disconnected (USB) serial-port causes infinite loop

[factoritbv]

In case of a USB CDC/ACM device, a file descriptor for the emulated serial port /dev/... is provided by an Unix based system.

In such a case, it can happen that the USB device is disconnected. This will cause that the select() that is executed with the corresponding file descriptor returns -1.

If this is happening, the code below, will end up in an infinite loop. Because the continue let the code-flow jump to the while(true), which then triggers another select() execution that again will return -1. Which triggers looping again.

jssc/src/main/cpp/_nix_based/jssc.cpp

Lines 537 to 559 in 1aff5f9

	/**
	* Waits until 'read()' has something to tell for the specified filedescriptor.
	*/
	static void awaitReadReady(JNIEnv*env, jlong fd){
	#if HAVE_POLL == 0
	// Alternative impl using 'select' as 'poll' isn't available (or broken).
	//assert(fd < FD_SETSIZE); // <- Might help when hunting SEGFAULTs.
	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;
	}
	// Did wait successfully.
	break;
	}
	FD_CLR(fd, &readFds);

Better would be if a exception is raised (the java way), however, if this breaks compatibility (like the comments in the code suggest), then at least awaitReadReady() should return an error code (in stead of void). The return value should be checked upon in the caller function of course, and should propagate to Java_jssc_SerialNativeInterface_readBytes() and trigger an immediate return. Again also here, proper handling should be notified towards the calling application. Simply the best way, would by using Java Exceptions.

Potential proposal for fixing the READ issue in Java_jssc_SerialNativeInterface_readBytes would be:

JNIEXPORT jbyteArray JNICALL Java_jssc_SerialNativeInterface_readBytes
  (JNIEnv *env, jobject object, jlong portHandle, jint byteCount){
    fd_set read_fd_set;
    jbyte *lpBuffer = new jbyte[byteCount];
    int byteRemains = byteCount;
    struct timeval timeout;
    int result;
    jclass threadClass = env->FindClass("java/lang/Thread");
    jmethodID areWeInterruptedMethod = env->GetStaticMethodID(threadClass, "interrupted", "()Z");
      
    while(byteRemains > 0) {

        // Check if the java thread has been interrupted, and if so, throw the exception
        if (env->CallStaticBooleanMethod(threadClass, areWeInterruptedMethod)) {
            jclass excClass = env->FindClass("java/lang/InterruptedException");
            env->ThrowNew(excClass, "Interrupted while reading from serial port");
            // It shouldn't matter what we return, the exception will be thrown right away
            break;
        }
        
        timeout.tv_sec = 0;
        timeout.tv_usec = 100000; // 100 ms
        FD_ZERO(&read_fd_set);
        FD_SET(portHandle, &read_fd_set);
        
        result = select(portHandle + 1, &read_fd_set, NULL, NULL, &timeout);
        if (result < 0) {
            env->ThrowNew(env->FindClass("java/io/IOException"), "Error while waiting for input from serial port");
            // Return value is ignored anyway, exception is handled immidiatly
            break;
        } else if (result == 0) {
            // timeout
            continue;
        }
        
        result = read(portHandle, lpBuffer + (byteCount - byteRemains), byteRemains);
        if(result < 0){
            env->ThrowNew(env->FindClass("java/io/IOException"), "Error reading from serial port");
            break;
        } else if (result == 0) {
            env->ThrowNew(env->FindClass("java/io/IOException"), "Serial port was closed unexpectedly");
            break;
        } else { // result > 0
            byteRemains -= result;
        }
    }
    returnArray = env->NewByteArray(byteCount);
    env->SetByteArrayRegion(returnArray, 0, byteCount, lpBuffer);
    delete[] lpBuffer;
    return returnArray;
}

Also important, with high throughput traffic, select() also should be used in the Java_jssc_SerialNativeInterface_writeBytes function. It is possible that the writes are done faster by the caller application, than the (kernel) output buffer and device speed can handle. Also the select() return value of -1 can be observed failures in writing to the serial-port.

Code proposal to improve the WRITE functionality in the Java_jssc_SerialNativeInterface_writeBytes function:

JNIEXPORT jboolean JNICALL Java_jssc_SerialNativeInterface_writeBytes
  (JNIEnv *env, jobject, jlong portHandle, jbyteArray buffer){
    jbyte* jBuffer = env->GetByteArrayElements(buffer, JNI_FALSE);
    jint bufferSize = env->GetArrayLength(buffer);
    fd_set write_fd_set;
    int byteRemains = bufferSize;
    struct timeval timeout;
    int result;
    jclass threadClass = env->FindClass("java/lang/Thread");
    jmethodID areWeInterruptedMethod = env->GetStaticMethodID(threadClass, "interrupted", "()Z");
      
    while(byteRemains > 0) {
        
        // Check if the java thread has been interrupted, and if so, throw the exception
        if (env->CallStaticBooleanMethod(threadClass, areWeInterruptedMethod)) {
            jclass excClass = env->FindClass("java/lang/InterruptedException");
            env->ThrowNew(excClass, "Interrupted while writing to serial port");
            // It shouldn't matter what we return, the exception will be thrown right away
            break;
        }
        
        timeout.tv_sec = 0;
        timeout.tv_usec = 100000; // 100 ms
        FD_ZERO(&write_fd_set);
        FD_SET(portHandle, &write_fd_set);
        
        result = select(portHandle + 1, NULL, &write_fd_set, NULL, &timeout);
        if (result < 0) {
            env->ThrowNew(env->FindClass("java/io/IOException"), "Waiting for serial port to become writable failed");
            // Return value is ignored anyway, exception is handled immidiatly
            break;
        } else if (result == 0) {
            // timeout
            continue;
        }
        
        result = write(portHandle, jBuffer + (bufferSize - byteRemains), byteRemains);
        if(result < 0){
            env->ThrowNew(env->FindClass("java/io/IOException"), "Error writing to serial port");
            break;
        } else if (result == 0) {
            env->ThrowNew(env->FindClass("java/io/IOException"), "Serial port was closed unexpectedly");
            break;
        } else { // result > 0
            byteRemains -= result;
        }
    }
    env->ReleaseByteArrayElements(buffer, jBuffer, 0);
    return JNI_TRUE; //result == bufferSize ? JNI_TRUE : JNI_FALSE;
}

[tresf]

@factoritbv thanks kindly for this. As a casual JNI user, I wasn't aware how to raise the exception. The "backwards compatibility" is simply a comment from @hiddenalpha's PR here: https://github.com/java-native/jssc/pull/90/files. The project has no issue with raising exceptions. We'd happily accept a PR which patches this behavior.

A bit of a disclaimer... this project has been on "life support" here for a while. Although I'm comfortable with the build system, I'm not very comfortable with the C++ portions and for that reason, improvements have been very slow to adopt here.

The fork was originally created to keep a dependant project alive and that continues to be my motivating factor. Unfortunately, this dependant project uses jssc sparingly, so focus on bugs usually is a direct result of issues over there.

So that's a long-winded way to say that we'd love these improvements incorporated and we're willing to help test them, but we need some help on the C++ side, preferably in the form of a PR. 🍻

[factoritbv]

@factoritbv thanks kindly for this. As a casual JNI user, I wasn't aware how to raise the exception. The "backwards compatibility" is simply a comment from @hiddenalpha's PR here: https://github.com/java-native/jssc/pull/90/files. The project has no issue with raising exceptions. We'd happily accept a PR which patches this behavior.

A bit of a disclaimer... this project has been on "life support" here for a while. Although I'm comfortable with the build system, I'm not very comfortable with the C++ portions and for that reason, improvements have been very slow to adopt here.

The fork was originally created to keep a dependant project alive and that continues to be my motivating factor. Unfortunately, this dependant project uses jssc sparingly, so focus on bugs usually is a direct result of issues over there.

So that's a long-winded way to say that we'd love these improvements incorporated and we're willing to help test them, but we need some help on the C++ side, preferably in the form of a PR. 🍻

Alright, we have created a PR. We experienced that with these improvement the library works very stable in a multi-threaded environment, handles serial port disconnects better and is much more user friendly because of the input/output stream add-on. We look forward to your test results.

[hiddenalpha]

Meanwhile jssc got some related fixes. They look very similar to the proposals here. Primarily #155 or #146 (But also by #152, #138, #137).

Do those solve this issue here?