Using `PyThreadState_SetAsyncExc` possibly causes deadlocks in downstream applications

[Delgan]

Hi.

I am the author of a Python library, and one user has reported bugs that occur when used in conjunction with Dramatiq (ref: Delgan/loguru#1335). Because this bug occurs sporadically, we haven't been able to identify a setup that reproduces it. However, the user investigated the issue and pinpointed a function in the Dramatic code that is likely related to the problem:

dramatiq/dramatiq/threading.py

Lines 79 to 88 in 9431762

	def _raise_thread_exception_cpython(thread_id, exception):
	exctype = (exception if inspect.isclass(exception) else type(exception)).__name__
	thread_id = thread_id_ctype(thread_id)
	exception = ctypes.py_object(exception)
	count = ctypes.pythonapi.PyThreadState_SetAsyncExc(thread_id, exception)
	if count == 0:
	logger.critical("Failed to set exception (%s) in thread %r.", exctype, thread_id.value)
	elif count > 1: # pragma: no cover
	logger.critical("Exception (%s) was set in multiple threads. Undoing...", exctype)
	ctypes.pythonapi.PyThreadState_SetAsyncExc(thread_id, ctypes.c_long(0))

More precisely, PyThreadState_SetAsyncExc is suspicious. This function is used to raise an Exception in another thread. However, this function is dangerous. Indeed, this can interrupt the thread at any point between two Python bytecode. The problem is that threading.Lock isn't robust enough to handle this use case. In some cases, it can result with locks being never released by the target thread, despite usage of idiomatic pattern such as a context manager.

I shared these concerns with Python core maintainers that confirmed this risk when using PyThreadState_SetAsyncExc. You can see a minimal reproducible example causing deadlock here (combining threading.Lock with PyThreadState_SetAsyncExc).

I'm not sure if this is a known issue for the Dramatiq maintainers. So I'm opening this ticket to at least let you know about it and find out your recommendations about it.

[LincolnPuzey]

Hi!

Thanks for the report and investigation. I was aware that PyThreadState_SetAsyncExc was a bit dangerous.
I don't think I knew about the specific risk of deadlocks.

#445 Is related, but is more about how the async exception can cause a internal dramatiq crash at an unexpected place.

There is also #560 and #578 which sound related by their title but might be a different issues entirely.

Unless CPython has a safer method to raise Exceptions in threads, I don't think there's any easy fix.

Brainstorming, we could write an alternative TimeLimit middleware that relies on user code periodically checking the limit, so no async Exception is required.