Skip to content

Commit

Permalink
Fix SEGV and deadlock when stopping async reads
Browse files Browse the repository at this point in the history
As of mercuryapi-1.31.4.35, when tm_reader_async.c's TMR_stopReading()
is called by Reader_stop_reading(), it ultimately ends up waiting for
reader->readState to become TMR_READ_STATE_DONE while holding the GIL.
This happens in tm_reader_async.c's do_background_reads(), but not until
the tag queue is emptied by read callbacks. These are triggered by
invoke_read_callback(), but this function blocks until it can acquire
the GIL. This frequently results in deadlock upon attempting to stop an
asynchronous read.

Previously, this was partially addressed by temporarily setting
self->readCallback (and self->statsCallback) to NULL in
Reader_stop_reading(), but this causes a SEGV in invoke_read_callback()
if it occurs after self->readCallback is verified to not be NULL but
before it is called.  Instead of temporarily setting these function
pointers to NULL, release the GIL in Reader_stop_reading(), allowing
invoke_read_callback() to empty the tag queue and thereby avoiding
deadlock. Then, use a mutex to prevent concurrent calls to
TMR_stopReading(), since this was previously prevented by the GIL.

Temporarily setting self->statsCallback to NULL in Reader_stop_reading()
also prevented stats callbacks from occurring in subsequent asynchronous
reads. This functionality is now fixed.
  • Loading branch information
charlescochran authored and gotthardp committed Feb 27, 2023
1 parent f9bf9c9 commit 325efef
Showing 1 changed file with 10 additions and 13 deletions.
23 changes: 10 additions & 13 deletions mercury.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ typedef struct {
PyObject *readCallback;
PyObject *statsCallback;
PyObject *exceptionCallback;
pthread_mutex_t stopReadingLock;
} Reader;

typedef struct {
Expand Down Expand Up @@ -161,6 +162,7 @@ Reader_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
return NULL;

self->tag_filter = NULL;
pthread_mutex_init(&self->stopReadingLock, NULL);

if ((ret = TMR_create(&self->reader, deviceUri)) != TMR_SUCCESS)
goto fail;
Expand Down Expand Up @@ -254,6 +256,7 @@ Reader_dealloc(Reader* self)
reset_filter(&self->tag_filter);
TMR_destroy(&self->reader);
Py_TYPE(self)->tp_free((PyObject*)self);
pthread_mutex_destroy(&self->stopReadingLock);
}

static
Expand Down Expand Up @@ -927,24 +930,18 @@ invoke_stats_callback(TMR_Reader *reader, const TMR_Reader_StatsValues *pdata, v
static PyObject *
Reader_stop_reading(Reader* self)
{
PyObject *temp = self->readCallback;
PyObject *temp2 = self->statsCallback;
TMR_Status ret;

/* avoid deadlock as calling stopReading will invoke the callback */
self->readCallback = NULL;
self->statsCallback = NULL;

if ((ret = TMR_stopReading(&self->reader)) != TMR_SUCCESS)
Py_BEGIN_ALLOW_THREADS // release the GIL to avoid deadlock
// use mutex to prevent concurrent calls to TMR_stopReading
pthread_mutex_lock(&self->stopReadingLock);
ret = TMR_stopReading(&self->reader);
pthread_mutex_unlock(&self->stopReadingLock);
Py_END_ALLOW_THREADS // reacquire the GIL
if (ret != TMR_SUCCESS)
{
self->readCallback = temp; /* revert back as the function will fail */
self->statsCallback = temp2;

PyErr_SetString(PyExc_RuntimeError, TMR_strerr(&self->reader, ret));
return NULL;
}

Py_XDECREF(temp);
Py_RETURN_NONE;
}

Expand Down

0 comments on commit 325efef

Please sign in to comment.