gotthardp / python-mercuryapi

Python wrapper for the ThingMagic Mercury API
MIT License
122 stars 63 forks source link

Fix SEGV and deadlock when stopping async reads #129

Closed charlescochran closed 1 year ago

charlescochran commented 2 years ago

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 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.

charlescochran commented 1 year ago

@gotthardp @natcl

Can we merge this? I think it is beneficial to the library, and it may fix the root cause of a few of the currently open issues. See this discussion for detailed explanations concerning a different but-related upstream bug which also exists.

gotthardp commented 1 year ago

@charlescochran Thank you! Looks reasonable, but I was not able to test it though.