Use a real read/write lock on the monitor thread.

THIS AND the previous commits are from
github.com/NeuronRobotics/pull/211/

The old `MonitorThreadLock` boolean field was only checked at a very
slow interval (5s!), and, itself not being synchronized, was prone to
race conditions. More importantly, it wasn't set during the monitor
thread's self-cleanup after hardware failure, so under typical access
patterns, the monitor thread and the application thread would both try
to clean up the monitor thread simultaneously. This race condition could
occasionally lead to a segfault (only reproduced on macOS, but I've no
doubt it could happen elsewhere).

I've also attempted to clean up some redundant flag fields, and
consolidate setting the remaining fields to further avoid
concurrency/reentrancy issues.

Author: MrDOS

src/main/c/src/SerialImp.c

@@ -3781,23 +3781,142 @@ JNIEXPORT void JNICALL RXTXPort(setflowcontrol)( JNIEnv *env,
 	return;
 }
 
-/*----------------------------------------------------------
-unlock_monitor_thread
+/**
+ * Write-lock the monitor thread to protect state mutation.
+ *
+ * Blocks until the write lock comes available.
+ *
+ * @param [in] env the JNI environment
+ * @param [in] obj an RXTXPort instance
+ * @return 0 on success; 1 on error
+ */
+int lock_monitor_thread(JNIEnv *env, jobject jobj)
+{
+	jfieldID monitorThreadStateWriteLockField = (*env)->GetFieldID(
+		env,
+		(*env)->GetObjectClass(env, jobj),
+		"monitorThreadStateWriteLock",
+		"Ljava/util/concurrent/locks/Lock;");
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
 
-   accept:      event_info_struct
-   perform:     unlock the monitor thread so event notification can start.
-   return:      none
-   exceptions:  none
-   comments:    Events can be missed otherwise.
-----------------------------------------------------------*/
+	jobject monitorThreadStateWriteLock = (*env)->GetObjectField(
+		env,
+		jobj,
+		monitorThreadStateWriteLockField);
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
 
-void unlock_monitor_thread( struct event_info_struct *eis )
+	jmethodID lock = (*env)->GetMethodID(
+		env,
+		(*env)->GetObjectClass(env, monitorThreadStateWriteLock),
+		"lock",
+		"()V");
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
+
+	(*env)->CallVoidMethod(env, monitorThreadStateWriteLock, lock);
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
+
+	return 0;
+}
+
+/**
+ * Signal that the monitor thread is ready for work.
+ *
+ * In order to signal the condition, the current thread must already hold the
+ * write lock.
+ *
+ * @param [in] env the JNI environment
+ * @param [in] obj an RXTXPort instance
+ * @return 0 on success; 1 on error
+ */
+int signal_monitor_thread_ready(JNIEnv *env, jobject jobj)
 {
-	JNIEnv *env = eis->env;
-	jobject jobj = *(eis->jobj);
+	jfieldID monitorThreadReadyField = (*env)->GetFieldID(
+		env,
+		(*env)->GetObjectClass(env, jobj),
+		"monitorThreadReady",
+		"Ljava/util/concurrent/locks/Condition;");
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
 
-	jfieldID jfid = (*env)->GetFieldID( env, (*env)->GetObjectClass( env, jobj ), "MonitorThreadLock", "Z" );
-	(*env)->SetBooleanField( env, jobj, jfid, (jboolean) 0 );
+	jobject monitorThreadReady = (*env)->GetObjectField(
+		env,
+		jobj,
+		monitorThreadReadyField);
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
+
+	jmethodID signal = (*env)->GetMethodID(
+		env,
+		(*env)->GetObjectClass(env, monitorThreadReady),
+		"signal",
+		"()V");
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
+
+	(*env)->CallVoidMethod(env, monitorThreadReady, signal);
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
+
+	return 0;
+}
+
+/**
+ * Unlock the write lock on the monitor thread to permit read and write access
+ * by other threads.
+ *
+ * In order to unlock the monitor thread, the current thread must already hold
+ * the write lock.
+ *
+ * @param [in] env the JNI environment
+ * @param [in] obj an RXTXPort instance
+ * @return 0 on success; 1 on error
+ */
+int unlock_monitor_thread(JNIEnv *env, jobject jobj)
+{
+	jfieldID monitorThreadStateWriteLockField = (*env)->GetFieldID(
+		env,
+		(*env)->GetObjectClass(env, jobj),
+		"monitorThreadStateWriteLock",
+		"Ljava/util/concurrent/locks/Lock;");
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
+
+	jobject monitorThreadStateWriteLock = (*env)->GetObjectField(
+		env,
+		jobj,
+		monitorThreadStateWriteLockField);
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
+
+	jmethodID unlock = (*env)->GetMethodID(
+		env,
+		(*env)->GetObjectClass(env, monitorThreadStateWriteLock),
+		"unlock",
+		"()V");
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
+
+	(*env)->CallVoidMethod(env, monitorThreadStateWriteLock, unlock);
+	if ((*env)->ExceptionCheck(env)) {
+		return 1;
+	}
+
+	return 0;
 }
 
 /*----------------------------------------------------------
@@ -4243,6 +4362,8 @@ RXTXPort.eventLoop
 ----------------------------------------------------------*/
 JNIEXPORT void JNICALL RXTXPort(eventLoop)( JNIEnv *env, jobject jobj )
 {
+	if (lock_monitor_thread(env, jobj)) goto end;
+
 #ifdef WIN32
 	int i = 0;
 #endif /* WIN32 */
@@ -4255,7 +4376,10 @@ JNIEXPORT void JNICALL RXTXPort(eventLoop)( JNIEnv *env, jobject jobj )
 	ENTER( "eventLoop\n" );
 	if ( !initialise_event_info_struct( &eis ) ) goto end;
 	if ( !init_threads( &eis ) ) goto end;
-	unlock_monitor_thread( &eis );
+
+	if (signal_monitor_thread_ready(env, jobj)) goto end;
+	if (unlock_monitor_thread(env, jobj)) goto end;
+
 	do{
 		report_time_eventLoop( );
 		do {
@@ -4271,13 +4395,18 @@ JNIEXPORT void JNICALL RXTXPort(eventLoop)( JNIEnv *env, jobject jobj )
 				 * drain loop). That will also set `eis.closing`, so the
 				 * function will return as usual in the next block.
 				 *
-				 * Note that `nativeavailable()` has thrown an exception at this
-				 * point, so we need to be careful about calling further JNI
-				 * functions. Conventional wisdom states that JNI functions
-				 * called when an exception is pending “may lead to unexpected
-				 * results”. Empirically, this seems to work okay; I suspect, at
-				 * worst, the functions might return an error. */
+				 * `nativeavailable()` has thrown an exception at this point, so
+				 * in order to call anything else which uses exceptions (like
+				 * the locking functions), we'll temporarily clear the exception
+				 * then reset it before continuing. */
+				jthrowable hardwareException = (*env)->ExceptionOccurred(env);
+				(*env)->ExceptionClear(env);
+
+				if (lock_monitor_thread(env, jobj)) goto end;
 				RXTXPort(interruptEventLoop)(env, jobj);
+				if (unlock_monitor_thread(env, jobj)) goto end;
+
+				(*env)->Throw(env, hardwareException);
 			}
 			/* nothing goes between this call and select */
 			if( eis.closing )
@@ -4962,7 +5091,22 @@ JNIEXPORT void JNICALL RXTXPort(interruptEventLoop)(JNIEnv *env,
 			usleep(1000);
 		}
 	}
+
 	index->eventloop_interrupted = 1;
+
+	jfieldID monThreadisInterruptedField = (*env)->GetFieldID(
+		env,
+		(*env)->GetObjectClass(env, jobj),
+		"monThreadisInterrupted",
+		"Z");
+	if ((*env)->ExceptionCheck(env)) {
+		return;
+	}
+	(*env)->SetBooleanField(env, jobj, monThreadisInterruptedField, JNI_TRUE);
+	if ((*env)->ExceptionCheck(env)) {
+		return;
+	}
+
 	/*
 	Many OS's need a thread running to determine if output buffer is
 	empty.  For Linux and Win32 it is not needed.  So closing is used to