This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Protection against using a Future with another loop only works with await
Type: behavior Stage: resolved
Components: asyncio Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: postponed
Dependencies: Superseder:
Assigned To: Nosy List: giampaolo.rodola, gvanrossum, pitrou, yselivanov
Priority: normal Keywords: patch

Created on 2017-11-06 17:24 by pitrou, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4319 merged pitrou, 2017-11-07 15:41
PR 4320 merged python-dev, 2017-11-07 16:04
Messages (16)
msg305662 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-06 17:24
If you await a Future with another loop than a loop at instance creation (this is trivial to do accidentally when using threads), asyncio raises a RuntimeError.  But if you use another part of the Future API, such as add_done_callback(), the situation isn't detected.

For example, this snippet will run indefinitely:

import asyncio
import threading


fut = asyncio.Future()

async def coro(loop):
    fut.add_done_callback(lambda _: loop.stop())
    loop.call_later(1, fut.set_result, None)
    while True:
        await asyncio.sleep(100000)

def run():
    loop = asyncio.new_event_loop()
    asyncio.set_event_loop(loop)
    loop.run_until_complete(coro(loop))
    loop.close()


t = threading.Thread(target=run)
t.start()
t.join()
msg305683 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-06 22:16
I'm not sure what the desired semantics for Futures and multiple loops is.  On the one hand, there is little point in having two event loops in the same thread at once (except for testing purposes).  On the other hand, the Future implementation is entirely not thread-safe (btw, the constructor optimistically claims the done callbacks are scheduled using call_soon_threadsafe(), but the implementation actually calls call_soon()).
msg305727 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-07 09:44
Guido, Yury, what is your take on this?  Do you think it would be fine for Future._schedule_callbacks() to check the event loop is the current one, or do you think it would impact performance too much (or perhaps it is simply not desirable)?
msg305761 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-11-07 14:31
> On the other hand, the Future implementation is entirely not thread-safe (btw, the constructor optimistically claims the done callbacks are scheduled using call_soon_threadsafe(), but the implementation actually calls call_soon()).

This is weird.  PEP 3156 specifies that Future uses call_soon.  The implementation uses call_soon.  And it actually makes sense to use call_soon for Futures.  

Situations when Future.cancel() or Future.set_result() is called from a different thread are extremely rare, so we want to avoid the overhead of using call_soon_threadsafe().  Moreover, I bet there are many other cases where Future implementation is not threadsafe.

If one absolutely needs to call Future.set_result() from a different thread, they can always do `loop.call_soon_threadsafe(fut.set_result, ...)`.

My opinion on this: update documentation for all Python versions to reflect that Future uses call_soon.

> Do you think it would be fine for Future._schedule_callbacks() to check the event loop is the current one, or do you think it would impact performance too much (or perhaps it is simply not desirable)?

I think we should update `Future._schedule_callbacks` to check if the loop is in debug mode.  If it is call `loop._check_thread()`, which will raise a RuntimeError if the current thread is different from the one that the loop belongs to.  

This will have no detectable overhead, but will ease debugging of edge cases like writing multithreaded asyncio applications.
msg305764 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-07 15:31
> My opinion on this: update documentation for all Python versions to reflect that Future uses call_soon.

Agreed.

> I think we should update `Future._schedule_callbacks` to check if the loop is in debug mode.

Unfortunately this is not sufficient for the snippet I posted.  The loop's thread_id is only set when the loop runs, but the main loop in that example never runs.
msg305765 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-07 15:33
My underlying question is why the Future has to set its loop in its constructor, instead of simply using get_event_loop() inside _schedule_callbacks().  This would always work.
msg305766 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-11-07 15:42
>> I think we should update `Future._schedule_callbacks` to check if the loop is in debug mode.

> Unfortunately this is not sufficient for the snippet I posted.  The loop's thread_id is only set when the loop runs, but the main loop in that example never runs.

If the loop isn't running, call_soon works just fine from any thread.  

call_soon_threadsafe is different from call_soon when the loop *is* running.  When it's running and blocked on IO, call_soon_threadsafe will make sure that the loop will be woken up.

Currently, _schedule_callbacks() calls loop.call_soon(), which already calls loop._check_thread().  So it looks like we don't need to change anything after all, right?
msg305767 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-07 15:46
The call_soon / call_soon_threadsafe distinction is not relevant to the problem I posted.  The problem is that the Future is registered with the event loop for the thread it was created in, even though it is only ever used in another thread (with another event loop).

Just try the snippet :-) If you want to see it finish in a finite time, move the future instantiation inside the coroutine.
msg305768 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-11-07 15:48
> My underlying question is why the Future has to set its loop in its constructor, instead of simply using get_event_loop() inside _schedule_callbacks().  This would always work.

So imagine a Future `fut` is completed. And we call `fut.add_done_callback()` in different contexts with different active event loops.  With your suggestion we'll schedule our callbacks in different loops.

Ideally you should use `loop.create_future()` when you can (and in libraries you usually can do that) to always make it explicit which loop your Future is attached to.
msg305769 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-07 15:52
> So imagine a Future `fut` is completed. And we call `fut.add_done_callback()` in different contexts with different active event loops.  With your suggestion we'll schedule our callbacks in different loops.

I'm wondering: does this situation occur in practice?  Since Future isn't threadsafe, is there really a point in using it from several loops at once?

> Ideally you should use `loop.create_future()` when you can (and in libraries you usually can do that) to always make it explicit which loop your Future is attached to.

Unfortunately that's not possible in our case.  Short version: we are using Tornado which creates a asyncio Future eagerly, see https://github.com/tornadoweb/tornado/blob/master/tornado/locks.py#L199
msg305770 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-11-07 15:57
> Just try the snippet :-) If you want to see it finish in a finite time, move the future instantiation inside the coroutine.

Yeah, I see the problem.  OTOH your proposed change to lazily attach a loop to the future isn't fully backwards compatible.  It would be a nightmare to find a bug in a large codebase caused by this change in Future behaviour.  So I'm -1 on this idea, that ship has sailed.

> Unfortunately that's not possible in our case.  Short version: we are using Tornado which creates a asyncio Future eagerly, see https://github.com/tornadoweb/tornado/blob/master/tornado/locks.py#L199

Maybe the solution is to fix Tornado?
msg305771 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-07 16:03
New changeset 22b1128559bdeb96907da5840960691bb050d11a by Antoine Pitrou in branch 'master':
bpo-31960: Fix asyncio.Future documentation for thread (un)safety. (#4319)
https://github.com/python/cpython/commit/22b1128559bdeb96907da5840960691bb050d11a
msg305772 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-07 16:09
> Maybe the solution is to fix Tornado?

That's a possibility.  I have to convince Ben Darnell that it deserves fixing :-)

Another possibility is to use the asyncio concurrency primitives on Python 3, though that slightly complicates things, especially as the various coroutines there don't take a timeout parameter.
msg305775 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-07 16:22
New changeset 518c6b97868d9c665475a40567b0aa417afad607 by Antoine Pitrou (Miss Islington (bot)) in branch '3.6':
bpo-31960: Fix asyncio.Future documentation for thread (un)safety. (GH-4319) (#4320)
https://github.com/python/cpython/commit/518c6b97868d9c665475a40567b0aa417afad607
msg305776 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-07 16:23
The documentation has been fixed.  Should we close this now?  Ideally I'd rather have asyncio warn me in such situations, but I feel this won't be doable.
msg305778 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-11-07 16:27
I guess you can set Resolution to "postponed", Stage to "Resolved".
History
Date User Action Args
2022-04-11 14:58:54adminsetgithub: 76141
2017-11-07 16:30:30pitrousetstatus: open -> closed
resolution: postponed
stage: resolved
2017-11-07 16:27:45yselivanovsetmessages: + msg305778
2017-11-07 16:23:11pitrousetmessages: + msg305776
2017-11-07 16:22:21pitrousetmessages: + msg305775
2017-11-07 16:09:50pitrousetmessages: + msg305772
stage: patch review -> (no value)
2017-11-07 16:04:38python-devsetstage: patch review
pull_requests: + pull_request4278
2017-11-07 16:03:32pitrousetmessages: + msg305771
2017-11-07 15:57:35yselivanovsetmessages: + msg305770
2017-11-07 15:52:07pitrousetmessages: + msg305769
2017-11-07 15:48:05yselivanovsetmessages: + msg305768
2017-11-07 15:46:17pitrousetmessages: + msg305767
2017-11-07 15:42:09yselivanovsetmessages: + msg305766
stage: patch review -> (no value)
2017-11-07 15:41:46pitrousetkeywords: + patch
stage: patch review
pull_requests: + pull_request4277
2017-11-07 15:33:46pitrousetmessages: + msg305765
2017-11-07 15:31:50vstinnersetnosy: - vstinner
2017-11-07 15:31:15pitrousetmessages: + msg305764
2017-11-07 14:31:53yselivanovsetmessages: + msg305761
2017-11-07 09:44:57pitrousetnosy: + gvanrossum
messages: + msg305727
2017-11-06 22:16:16pitrousetmessages: + msg305683
2017-11-06 17:24:09pitroucreate