classification
Title: asyncio: raise an exception when called from the wrong thread
Type: Stage:
Components: asyncio Versions: Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: gvanrossum, python-dev, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2014-11-23 16:38 by vstinner, last changed 2014-12-26 20:47 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
asyncio_thread.patch vstinner, 2014-11-28 13:11
asyncio_thread-2.patch vstinner, 2014-12-05 09:11 review
check_thread.patch vstinner, 2014-12-17 23:21 review
check_thread-2.patch vstinner, 2014-12-20 23:41 review
bench_call_soon.py vstinner, 2014-12-20 23:41
check_thread-3.patch vstinner, 2014-12-25 23:19 review
Messages (19)
msg231569 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-11-23 16:38
The call_soon(), call_later() and call_at() methods of asyncio event loops should raise an exception in debug code when they are not called from the right thread.

Currently, BaseEventLoop._assert_is_current_event_loop() does nothing if the event loop policy has no event loop for the current thread, when get_event_loop() raises an AssertionError.
msg231811 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-11-28 13:11
Here is a patch without unit:

- modify get_event_loop() to always raise a RuntimeError if the thread has no event loop. Before an AssertionError was not raised if python runs with -O option
- modify BaseEventLoop._assert_is_current_event_loop() to fail if the thread has an event loop
- modify tests to set the event loop to the newly created event loop, instead of setting it to None
msg232166 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-05 00:47
Guido, Yury: What do you think? Does it sound reasonable to raise an exception if an event loop is used from the wrong thread?
msg232172 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-12-05 01:32
Maybe only in debug mode? Getting thread ID or current thread may be
expensive. Also it is conceivable that run... is called first from one
thread and then from another. Maybe.
On Dec 4, 2014 4:47 PM, "STINNER Victor" <report@bugs.python.org> wrote:

>
> STINNER Victor added the comment:
>
> Guido, Yury: What do you think? Does it sound reasonable to raise an
> exception if an event loop is used from the wrong thread?
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22926>
> _______________________________________
>
msg232173 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-12-05 03:21
> - modify tests to set the event loop to the newly created event loop, instead of setting it to None

I'm not sure that this particular change is a great idea. I kind of liked that unittests ensure that loop is passed everywhere explicitly in asyncio.


> - modify get_event_loop() to always raise a RuntimeError if the thread has no event loop. Before an AssertionError was not raised if python runs with -O option

+1.


> - modify BaseEventLoop._assert_is_current_event_loop() to fail if the thread has an event loop

Hm, I think I don't understand why this function doesn't to anything when there is no loop in the thread... Let's fix it. In your patch, you should also replace 'except AsserionError' with 'except RuntimeError'.


> Does it sound reasonable to raise an exception if an event loop is used from the wrong thread?

I think we should in debug mode at least.


> Getting thread ID or current thread may be expensive.

Victor, can you benchmark this?  I'm pretty sure that Guido is right about this, but sometimes syscalls are pretty fast.  I doubt that a lot of people know about debug mode, so if it has a negligible cost I'd do the check every time.
msg232174 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-12-05 03:30
On Thu, Dec 4, 2014 at 7:21 PM, Yury Selivanov <report@bugs.python.org>
wrote:

>
> Yury Selivanov added the comment:
>
> > - modify tests to set the event loop to the newly created event loop,
> instead of setting it to None
>
> I'm not sure that this particular change is a great idea. I kind of liked
> that unittests ensure that loop is passed everywhere explicitly in asyncio.
>

Indeed, don't break that.

>
> > - modify get_event_loop() to always raise a RuntimeError if the thread
> has no event loop. Before an AssertionError was not raised if python runs
> with -O option
>
> +1.
>

+1

>
> > - modify BaseEventLoop._assert_is_current_event_loop() to fail if the
> thread has an event loop
>
> Hm, I think I don't understand why this function doesn't to anything when
> there is no loop in the thread... Let's fix it. In your patch, you should
> also replace 'except AsserionError' with 'except RuntimeError'.
>
>
> > Does it sound reasonable to raise an exception if an event loop is used
> from the wrong thread?
>
> I think we should in debug mode at least.
>
>
> > Getting thread ID or current thread may be expensive.
>
> Victor, can you benchmark this?  I'm pretty sure that Guido is right about
> this, but sometimes syscalls are pretty fast.  I doubt that a lot of people
> know about debug mode, so if it has a negligible cost I'd do the check
> every time.
>

I take it back, it's only 250 nsec to call threading.current_thread(), and
300 nsec to acquire and release a lock.

I still think you shouldn't set the thread when the loop is created but
only when run*() is active. (Using call_later() from another thread when
the loop is inactive should not be an error IMO -- there's nothing against
passing a selector instance to another thread as long as only one thread at
a time waits for it.)
msg232182 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-05 09:11
Ok, here is an updated patch (version 2):

- tests still ensure that the event loop is passed explicitly
- call_soon() & cie now fail if the policy has no event loop set for the current thread (before it only failed if the event loop set in the policy was different than the called event loop)

> I'm not sure that this particular change is a great idea. I kind of liked that unittests ensure that loop is passed everywhere explicitly in asyncio.

I agree, but we can just monkey-patch the _assert_is_current_event_loop() to disable the check even if debug mode. For example, it can be done in test_utils.TestCase.set_event_loop(), no need to modify individual test cases.

> Maybe only in debug mode?

Oh sorry, I forgot to mention that my patch only enhance an existing test, _assert_is_current_event_loop(), which is only done in debug mode.

I'm so happy to have this debug mode, at the beginning Guido didn't want it :-D

> - modify BaseEventLoop._assert_is_current_event_loop() to fail if the thread has an event loop

Oops, the method already fails if the event loop is different than the current event loop of the policy. My patch changes the behaviour when the policy has *no* current event loop.

> In your patch, you should also replace 'except AsserionError' with 'except RuntimeError'.

Ok, that's why we must reject any patch without unit test :-D

> I still think you shouldn't set the thread when the loop is created but
only when run*() is active. (Using call_later() from another thread when
the loop is inactive should not be an error IMO -- there's nothing against
passing a selector instance to another thread as long as only one thread at
a time waits for it.)

Hum, I'm not fully convinced. Technically, it "works" to call call_soon() instead of call_soon_threadsafe() when the event loop is not running yet. But multithreading means race condition. It probably means that in most cases, calling call_soon() would work because the event loop didn't start yet, but in some cases, the event loop may already run depending on the timing.

I prefer to force users to always use call_soon_threadsafe() to avoid bad suprises.
msg232229 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-12-06 01:01
Hi Victor,

I left you some feedback in code review.

I'm kind of leaning towards your proposal that we should force users to always use safe API. But I also understand Guido's point.
msg232258 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-12-06 22:52
I'm okay with this approach now.
msg232774 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-16 23:48
Guido wrote:
> I'm okay with this approach now.

I'm not sure that I understood your opinion. Are you ok to raise an exception in debug mode if call_soon() is called from a thread which has no event loop attached?

(Currently, an exception is already raised in debug mode, but only if the thread has an attached event loop, but not the good one.)
msg232777 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-12-16 23:53
Well, the PEP clearly states that get_event_loop() should never return None
and raise an exception if the context has no environment. (Where context ~~
thread.)

On Tue, Dec 16, 2014 at 3:48 PM, STINNER Victor <report@bugs.python.org>
wrote:
>
>
> STINNER Victor added the comment:
>
> Guido wrote:
> > I'm okay with this approach now.
>
> I'm not sure that I understood your opinion. Are you ok to raise an
> exception in debug mode if call_soon() is called from a thread which has no
> event loop attached?
>
> (Currently, an exception is already raised in debug mode, but only if the
> thread has an attached event loop, but not the good one.)
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22926>
> _______________________________________
>
msg232782 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-17 00:21
2014-12-17 0:53 GMT+01:00 Guido van Rossum <report@bugs.python.org>:
> Well, the PEP clearly states that get_event_loop() should never return None
> and raise an exception if the context has no environment. (Where context ~~
> thread.)

You don't reply to my question, or I misunderstood your anwer.
call_soon() is a method an event loop, there is no need to call
get_event_loop(). get_event_loop() is only called in debug mode to
help the developer to detect obvious bugs. I don't think that the
behaviour of the debug mode must be written in the PEP, it's more
implementation specific.
msg232786 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-12-17 00:31
Oh sorry. I'm in too much of a hurry. :-(

The problem is that the thread may not have a loop, but the loop still has
a tread. E.g. the tests intentionally call set_event_loop(None) to make
sure that no code depends on the implict event loop. An app should be
allowed to do the same, even in debug mode.

IIUC this is already broken in debug mode? (Or perhaps only in non-main
threads?)

So in order to do correct diagnostics here we would have to add an "owning
thread" pointer to every event loop; then call_later() and friends can
check that the owning thread is the same as the current thread. But I'm not
sure that this is worth the work. (And I could see issues with the owning
thread concept as well.)

On Tue, Dec 16, 2014 at 4:21 PM, STINNER Victor <report@bugs.python.org>
wrote:
>
>
> STINNER Victor added the comment:
>
> 2014-12-17 0:53 GMT+01:00 Guido van Rossum <report@bugs.python.org>:
> > Well, the PEP clearly states that get_event_loop() should never return
> None
> > and raise an exception if the context has no environment. (Where context
> ~~
> > thread.)
>
> You don't reply to my question, or I misunderstood your anwer.
> call_soon() is a method an event loop, there is no need to call
> get_event_loop(). get_event_loop() is only called in debug mode to
> help the developer to detect obvious bugs. I don't think that the
> behaviour of the debug mode must be written in the PEP, it's more
> implementation specific.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22926>
> _______________________________________
>
msg232833 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-17 23:21
Guido> So in order to do correct diagnostics here we would have to add an "owning
thread" pointer to every event loop;

I didn't understand why this approach was not taken when the check was introduced. I was surprised that get_event_loop() was used for the check instead.

Here is a patch checking the thread using threading.get_ident(). The check still works when set_event_loop(None) is used in unit tests.

I created the issue #23074 for the change: get_event_loop() must always raise an exception, even when assertions are disabled by -O.

--

asyncio requires thread support, it is already the case without my patch. For example, the event loop policy uses threading.Local.

In the aioeventlet project, call_soon() writes a byte in the self pipe (to wake up the selector) if the selector is waiting for events; call_soon() is "green thread safe". It is less efficient than calling forcing users to call explicitly call_soon_threadsafe(), but aioeventlet is written to be compatible with asyncio and eventlet, while these projects are very different.

In the aiogevent and aioeventlet projects, I also store the current greenlet, but for a different purpose: ensure that yield_future() is not called in the greenlet running the event loop, which would blocking.
msg232985 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-20 23:41
Rebased patch which now always check the thread, even in release mode.

Summary of the current version of the patch (check_thread-2.patch):

- call_soon/call_at now checks if they are called from the thread running the event loop
- the check is only done when the event loop in running
- the check is always done, in relase and debug mode (we may only do it in release mode if you consider that the overhead is too high, see benchmark numbers below)
- add a unit test for the thread check
- since the check is only done when the event loop in running, we don't need the hack to avoid the thread check in the proactor event loop, for "self._call_soon(self._loop_self_reading, ())"
- replace the _running attribute with a new _owner attribute in BaseEventLoop


> Victor, can you benchmark this?

Here is a benchmark for call_soon(): bench_call_soon.py. It computes the performance of one call to call_soon() in average. It uses 10,000 iterations, the test is run 5 times and the minimum timing is displayed.

Since a call to traceback.extract_stack() takes more than 65 us, whereas a call to call_soon() in release mode takes 2.8 us, I removed the call in Handle constructor for the benchmark to focus on the thread check.

- asyncio without extract_stack(), in release mode: 2491 ns per call
- asyncio without extract_stack(), in debug mode: 3588 ns per call (1.4x slower, +1097 ns)
- asyncio without extract_stack(), with check_thread-2.patch: 2721 ns per call (1.1x slower, +230 ns)

The overhead of check_thread-2.patch is +230 ns (1.1x slower) per call to call_soon().

Do you consider that the overhead is low enough to run the check even in release mode?

At least on Linux, threading.get_ident() is not a system call. Performances may be different on other platforms (ex: BSD, Windows).

In check_thread-2.patch, I "inlined" the thread check directly in call_at() and call_soon(). For performance, but also because the check only takes 2 lines, and so the error message can contain the function name (call_soon/call_at).
msg233098 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-25 23:19
I'm going to commit check_thread-3.patch if nobody complains: in debug mode, call_soon(), call_at() and call_later() now check threading.get_ident() to ensure that they are called from the thread running the event loop.
msg233103 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-12-26 02:08
SGTM. Merry Xmas!
msg233116 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-12-26 20:46
New changeset 4e8ac4173b3c by Victor Stinner in branch '3.4':
Issue #22926: In debug mode, call_soon(), call_at() and call_later() methods of
https://hg.python.org/cpython/rev/4e8ac4173b3c
msg233117 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-26 20:47
Thanks for the review. I commited my change in Tulip, Python 3.4 & 3.5.
History
Date User Action Args
2014-12-26 20:47:31vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg233117
2014-12-26 20:46:40python-devsetnosy: + python-dev
messages: + msg233116
2014-12-26 02:08:27gvanrossumsetmessages: + msg233103
2014-12-25 23:20:00vstinnersetfiles: + check_thread-3.patch

messages: + msg233098
2014-12-20 23:41:15vstinnersetfiles: + bench_call_soon.py
2014-12-20 23:41:07vstinnersetfiles: + check_thread-2.patch

messages: + msg232985
2014-12-17 23:21:36vstinnersetfiles: + check_thread.patch

messages: + msg232833
2014-12-17 00:31:25gvanrossumsetmessages: + msg232786
2014-12-17 00:21:17vstinnersetmessages: + msg232782
2014-12-16 23:53:31gvanrossumsetmessages: + msg232777
2014-12-16 23:48:40vstinnersetmessages: + msg232774
2014-12-06 22:52:01gvanrossumsetmessages: + msg232258
2014-12-06 01:01:03yselivanovsetmessages: + msg232229
2014-12-05 09:11:41vstinnersetfiles: + asyncio_thread-2.patch

messages: + msg232182
2014-12-05 03:30:48gvanrossumsetmessages: + msg232174
2014-12-05 03:21:52yselivanovsetmessages: + msg232173
2014-12-05 01:32:50gvanrossumsetmessages: + msg232172
2014-12-05 00:47:01vstinnersetmessages: + msg232166
2014-11-28 13:11:47vstinnersetfiles: + asyncio_thread.patch
keywords: + patch
messages: + msg231811
2014-11-23 16:39:17vstinnersettitle: asyncio: -> asyncio: raise an exception when called from the wrong thread
2014-11-23 16:38:14vstinnercreate