Protection against using a Future with another loop only works with await

[pitrou]

BPO	31960
Nosy	@gvanrossum, @pitrou, @giampaolo, @1st1
PRs	bpo-31960: Fix asyncio.Future documentation for thread (un)safety. #4319 [3.6] bpo-31960: Fix asyncio.Future documentation for thread (un)safety. (GH-4319) #4320

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-11-07.16:30:30.856>
created_at = <Date 2017-11-06.17:24:09.546>
labels = ['type-bug', '3.7', 'expert-asyncio']
title = 'Protection against using a Future with another loop only works with await'
updated_at = <Date 2017-11-07.16:30:30.855>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2017-11-07.16:30:30.855>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2017-11-07.16:30:30.856>
closer = 'pitrou'
components = ['asyncio']
creation = <Date 2017-11-06.17:24:09.546>
creator = 'pitrou'
dependencies = []
files = []
hgrepos = []
issue_num = 31960
keywords = ['patch']
message_count = 16.0
messages = ['305662', '305683', '305727', '305761', '305764', '305765', '305766', '305767', '305768', '305769', '305770', '305771', '305772', '305775', '305776', '305778']
nosy_count = 4.0
nosy_names = ['gvanrossum', 'pitrou', 'giampaolo.rodola', 'yselivanov']
pr_nums = ['4319', '4320']
priority = 'normal'
resolution = 'postponed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue31960'
versions = ['Python 3.6', 'Python 3.7']

[pitrou]

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()

[pitrou]

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

[pitrou]

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)?

[1st1]

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.

[pitrou]

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.

[pitrou]

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.

[1st1]

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?

[pitrou]

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.

[1st1]

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.

[pitrou]

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

[1st1]

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?

[pitrou]

New changeset 22b1128 by Antoine Pitrou in branch 'master':
bpo-31960: Fix asyncio.Future documentation for thread (un)safety. (bpo-4319)
22b1128

[pitrou]

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.

[pitrou]

New changeset 518c6b9 by Antoine Pitrou (Miss Islington (bot)) in branch '3.6':
bpo-31960: Fix asyncio.Future documentation for thread (un)safety. (GH-4319) (bpo-4320)
518c6b9

[pitrou]

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.

[1st1]

I guess you can set Resolution to "postponed", Stage to "Resolved".