classification
Title: Detect accessing event loop from a different thread outside of _debug
Type: enhancement Stage:
Components: asyncio Versions: Python 3.8, Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, hniksic, yselivanov
Priority: normal Keywords:

Created on 2018-05-22 19:00 by hniksic, last changed 2018-05-24 07:09 by hniksic.

Messages (6)
msg317324 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2018-05-22 19:00
Looking at StackOverflow's python-asyncio tag[1], it appears that it's a very common mistake for users to invoke asyncio functions or methods from a thread other than the event loop thread. In some cases this happens because the user is careless with threads and hasn't read the documentation. But in many cases what happens is that a third-party library invoked a callback in a different thread without making it transparent that that's what it's doing.

The trouble is that in many cases doing these things, e.g. calling loop.call_soon() or loop.create_task() from the wrong thread, will *appear to work*. The typical event loop is busy servicing different coroutines, so a function or task enqueued without a proper wakeup gets picked up soon enough. This is, of course, a disaster waiting to happen because it could easily lead to corruption of event loop's data structures. But users don't know that, and many of them become aware of the problem only after wondering "why does my code start working when I add a coroutine that does nothing but asyncio.sleep(0.1) in an infinite loop?" Some may never even fix their code, just assuming that asyncio takes a long time to process a new task or something like that.

I suggest that asyncio should be stricter about this error and that methods and functions that operate on the event loop, such as call_soon, call_later, create_task, ensure_future, and close, should all call _check_thread() even when not in debug mode. _check_thread() warns that it "should only be called when self._debug == True", hinting at "performance reasons", but that doesn't seem justified. threading.get_ident() is efficiently implemented in C, and comparing that integer to another cached integer is about as efficient an operation as it gets.

The added benefit would be a vast improvement of robustness of asyncio-based programs, saving many hours of debugging.


[1]
Here is an incomplete list of questions where the users stumbled on this problem, and that's only from the last three months or so:

https://stackoverflow.com/questions/49906034/python-asyncio-run-forever-and-tasks
https://stackoverflow.com/questions/49851514/python-websockets-and-gtk-confused-about-asyncio-queue
https://stackoverflow.com/questions/49533612/using-asyncio-loop-reference-in-another-file
https://stackoverflow.com/questions/49093623/strange-behaviour-when-task-added-to-empty-loop-in-different-thread
https://stackoverflow.com/questions/48836285/python-asyncio-event-wait-not-responding-to-event-set
https://stackoverflow.com/questions/48833644/how-to-wait-for-asynchronous-callback-in-the-background-i-e-not-invoked-by-us
https://stackoverflow.com/questions/48695670/running-asynchronous-code-synchronously-in-separate-thread
msg317340 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-05-22 20:19
> I suggest that asyncio should be stricter about this error and that methods and functions that operate on the event loop, such as call_soon, call_later, create_task, ensure_future, and close, should all call _check_thread() even when not in debug mode. _check_thread() warns that it "should only be called when self._debug == True", hinting at "performance reasons", but that doesn't seem justified. threading.get_ident() is efficiently implemented in C, and comparing that integer to another cached integer is about as efficient an operation as it gets.

I'd be OK with this if the performance penalty is within 0.5% in microbenchmarks for asyncio & uvloop.
msg317435 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2018-05-23 18:39
> I'd be OK with this if the performance penalty is within 0.5% in microbenchmarks for asyncio & uvloop.

@yselivanov Are you referring to specific microbenchmarks published somewhere, or the general "echo server" style microbenchmarks?
msg317436 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-05-23 18:41
Echo server is usually a good enough microbenchmark (you can use benchmarks from uvloop/examples).  Repeatedly calling call_soon would also be an interesting micro-benchmark but less important (unless it shows that call_soon is 2x slower or something).
msg317446 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-05-23 20:08
Heh, should we sacrifice performance?
Documentation for asyncio explicitly states that the only safe interthreading call is `call_soon_threadsafe()`.

If people use multithreaded environments with asyncio (Why? Usually `run_in_executor()` doesn't require communication with loop. Maybe they trying to do strange things like fetching web pages using aiohttp from Django application?) -- they should be aware of problems and consequences.

Like threaded programming is potentially dangerous without locks and other things.

I very doubt that correct asyncio program have problems like this. In opposite `call_soon()` is maybe the hottest path of asyncio, it should be as fast as possible. Any slowdown is not desirable.

P.S. I believe teaching people to get rid of `run_until_complete()` but switching to `asyncio.run()` can help better than anything else.

P.P.S. If a user wants to shoot in own leg -- nobody can stop it. Don't underestimate people's inventiveness.
msg317540 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2018-05-24 07:09
I would definitely not propose or condone sacrificing performance.

Part of the reason why I suggested the check is that it can be done efficiently - it is literally a comparison of two integers, both of which are obtained trivially. I would hope that it doesn't affect performance at all, but only measurements will show.

I looked at the implementation of threading.get_ident(), and it looks just as one would hope - a cast of pthread_self() to unsigned long, and a call to PyLong_FromUnsignedLong. If needed, it might be further improved by caching the resulting PyObject * in a __thread variable on platforms that support them, but hopefully that is not necessary. (It would require additional code to Py_CLEAR the cached PyObject * when the thread exits.) 

> I very doubt that correct asyncio program have problems like this.

That is true by definition, since what we're discussing is incorrect in asyncio. But we could be better at diagnosing the incorrect use, and we could do so (or so I hope) very efficiently. Failure to emit a diagnostic leads to bugs that are discovered much later or never.

As for why people use multi-threading with asyncio, keep in mind that many libraries create threads behind the scenes without the user being aware of it. Like me, you are a regular at python-asyncio tag, so you've seen those. In some cases you can write it off as the user doing something stupid and not reading the docs, but in many others, the mistake is far from obvious, especially for users who are not (yet) asyncio experts.
History
Date User Action Args
2018-05-24 07:09:17hniksicsetmessages: + msg317540
2018-05-23 20:08:36asvetlovsetmessages: + msg317446
2018-05-23 18:41:48yselivanovsetmessages: + msg317436
2018-05-23 18:39:13hniksicsetmessages: + msg317435
2018-05-22 20:19:53yselivanovsetmessages: + msg317340
2018-05-22 19:00:04hniksiccreate