Skip to content

Commit

Permalink
Use a real read/write lock on the monitor thread.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
MrDOS authored and Claudia Pellegrino committed Feb 8, 2024
1 parent 4caae7b commit 2daf6da
Show file tree
Hide file tree
Showing 2 changed files with 280 additions and 145 deletions.
184 changes: 164 additions & 20 deletions src/main/c/src/SerialImp.c
Original file line number Diff line number Diff line change
Expand Up @@ -3772,23 +3772,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;
}

/*----------------------------------------------------------
Expand Down Expand Up @@ -4229,6 +4348,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 */
Expand All @@ -4241,7 +4362,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 {
Expand All @@ -4257,13 +4381,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 )
Expand Down Expand Up @@ -4952,7 +5081,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
Expand Down
Loading

0 comments on commit 2daf6da

Please sign in to comment.