Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add run_coroutine_threadsafe() to asyncio #69491

Closed
gvanrossum opened this issue Oct 3, 2015 · 23 comments
Closed

Add run_coroutine_threadsafe() to asyncio #69491

gvanrossum opened this issue Oct 3, 2015 · 23 comments
Assignees
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@gvanrossum
Copy link
Member

BPO 25304
Nosy @gvanrossum, @vstinner, @bitdancer, @1st1, @vxgmichel
Files
  • ensure_future_threadsafe.py: ensure_future_threadsafe implementation
  • run_coroutine_threadsafe_doc.patch: run_coroutine_threadsafe documentation
  • run_coroutine_threadsafe_2.patch: run_couroutine_threadsafe patch 2 (code, test and docs)
  • 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 = 'https://github.com/gvanrossum'
    closed_at = <Date 2015-10-05.23:27:47.135>
    created_at = <Date 2015-10-03.15:23:53.631>
    labels = ['type-bug', 'expert-asyncio']
    title = 'Add run_coroutine_threadsafe() to asyncio'
    updated_at = <Date 2015-11-18.17:45:43.421>
    user = 'https://github.com/gvanrossum'

    bugs.python.org fields:

    activity = <Date 2015-11-18.17:45:43.421>
    actor = 'yselivanov'
    assignee = 'gvanrossum'
    closed = True
    closed_date = <Date 2015-10-05.23:27:47.135>
    closer = 'gvanrossum'
    components = ['asyncio']
    creation = <Date 2015-10-03.15:23:53.631>
    creator = 'gvanrossum'
    dependencies = []
    files = ['40672', '40689', '40691']
    hgrepos = []
    issue_num = 25304
    keywords = ['patch']
    message_count = 23.0
    messages = ['252215', '252217', '252218', '252267', '252269', '252270', '252272', '252277', '252286', '252287', '252293', '252334', '252336', '252343', '252344', '252345', '252348', '252362', '252365', '252368', '252369', '254847', '254850']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'vstinner', 'r.david.murray', 'Yury.Selivanov', 'python-dev', 'yselivanov', 'vxgmichel']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue25304'
    versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']

    @gvanrossum
    Copy link
    Member Author

    This is a placeholder bug to reference the PR: python/asyncio#273 by Vincent Michel.

    @gvanrossum gvanrossum self-assigned this Oct 3, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 3, 2015

    New changeset 25e05b3e1869 by Guido van Rossum in branch '3.4':
    Issue bpo-25304: Add asyncio.run_coroutine_threadsafe(). By Vincent Michel.
    https://hg.python.org/cpython/rev/25e05b3e1869

    New changeset e0db10d8c95e by Guido van Rossum in branch '3.5':
    Issue bpo-25304: Add asyncio.run_coroutine_threadsafe(). By Vincent Michel. (Merge 3.4->3.5.)
    https://hg.python.org/cpython/rev/e0db10d8c95e

    New changeset 69829a7fccde by Guido van Rossum in branch 'default':
    Issue bpo-25304: Add asyncio.run_coroutine_threadsafe(). By Vincent Michel. (Merge 3.5->3.6.)
    https://hg.python.org/cpython/rev/69829a7fccde

    @gvanrossum
    Copy link
    Member Author

    It's done. But I am hoping you (or someone) will add docs.

    @gvanrossum gvanrossum added the type-bug An unexpected behavior, bug, or error label Oct 3, 2015
    @vxgmichel
    Copy link
    Mannequin

    vxgmichel mannequin commented Oct 4, 2015

    While I was working on the documentation update, I realized that what we called run_coroutine_threadsafe is actually a thread-safe version of ensure_future. What about renaming it to ensure_future_threadsafe? It might be a bit late since run_coroutine_threadsafe has been committed, but I think it is worth to be considered. I can see two benefits:

    • it is less confusing, because it has the same name and using the same prototype as ensure_future
    • it accepts futures and awaitables

    The docstring would be "Wrap a coroutine, an awaitable or a future in a concurrent.futures.Future.". The documentation would explain that it works like ensure_future except:

    1. its execution is threadsafe
    2. it provides a thread-safe future instead of a regular future

    I attached an implementation of it. Also, note that I added a try-except in the callback, which is not mandatory but probably a good thing have.

    In any case, I'll keep working on the documentation update.

    @1st1
    Copy link
    Member

    1st1 commented Oct 4, 2015

    • it is less confusing, because it has the same name and using the same prototype as ensure_future
    • it accepts futures and awaitables

    I like this idea.

    @gvanrossum
    Copy link
    Member Author

    I'm against that idea. I don't really see a great important future for this method either way: It's just a little bit of glue between the threaded and asyncio worlds, and people will learn how to use it by finding an example.

    The existing ensure_future() function is mostly meant as an internal helper, with the understanding that libraries written on top of asyncio might also need this functionality. Basically I want people writing asyncio code not to have to worry about the difference between futures and coroutines, because they can both be awaited for; ensure_future() helps preserve that illusion for code that really needs a future (usually to add a callback).

    But honestly I *don't* want to encourage flipping back and forth between threads and event loops; I see it as a necessary evil. The name we currently have is fine from the POV of someone coding in the threaded world who wants to hand off something to the asyncio world.

    Why would someone in the threaded world have an asyncio.future that they need to wait for? That sounds like they're mixing up the two worlds -- or they should be writing asyncio code instead of threaded code.

    OTOH, I would love to see the documentation update!

    @bitdancer
    Copy link
    Member

    I thought you might be interested to know what the name suggests to a relative newcomer to asyncio.

    When I saw this issue opened, I though "oh, good, an easy way to submit a coroutine from a thread in my test code, now I don't have to write 'loop.call_soon_threadsafe(loop.create_task, my_coroutine(loop=loop))". Which I suspect you are going to tell me is stupid code, but the point is that that's what the name 'call_coroutine_threadsafe' made me think it was going to do. After reading the linked issue I was very confused about what it was actually going to do, as I've never gotten around to looking up what ensure_future does.

    If I'm understanding correctly now, I think my assumption was pretty much correct, and the only difference is that I get back something I can actually manipulate if I need to, making it even better, and which the documentation should make clear when I get to read it :)

    @YurySelivanov
    Copy link
    Mannequin

    YurySelivanov mannequin commented Oct 4, 2015

    But honestly I *don't* want to encourage flipping back and forth between threads and event loops; I see it as a necessary evil. The name we currently have is fine from the POV of someone coding in the threaded world who wants to hand off something to the asyncio world.

    I agree, but on the other hand supporting awaitables in addition to coroutines would be great. Can we at least add that?

    @gvanrossum
    Copy link
    Member Author

    @rdm: thanks, you nailed it. :-)

    @yury: but where would you have gotten the awaitable in the first place? It's easy to see how to get a coroutine -- just define it (with either @coroutine or async def) and call it -- and I think that's the only use case that matters here.

    I'm unclear how you could have gotten another awaitable (e.g. an asyncio.Future instance) in another thread -- except by an utterly confused user.

    @1st1
    Copy link
    Member

    1st1 commented Oct 4, 2015

    @yury: but where would you have gotten the awaitable in the first place? It's easy to see how to get a coroutine -- just define it (with either @coroutine or async def) and call it -- and I think that's the only use case that matters here.

    Just a few days ago Andrew Svetlov raised a question on github -- how could he refactor aiohttp.get() coroutine to be a coroutine (compatible with coroutine ABC) and an async context manager. With my recent commit, ensure_future() now accepts not just coroutines or futures, but all objects with __await__.

    So if you have a library with an API exposed through coroutines, it would be great if you have some freedom do refactor it and keep it compatible with asyncio functions such as run_coroutine_threadsafe()

    @gvanrossum
    Copy link
    Member Author

    Well, I still worry that this is just going to encourage more people to try and call awaitables from threaded code, and through some circuitous path of misunderstandings (probably involving StackOverflow :-) end up using this function.

    (Pretty much the worst-case scenario would be calling run_coroutine_threadsafe() targeting the *current* loop, and then blocking the thread waiting for its result. Deadlock!)

    @vxgmichel
    Copy link
    Mannequin

    vxgmichel mannequin commented Oct 5, 2015

    I attached the first version of the documentation for run_coroutine_threadsafe. The Concurrency and multithreading section also needs to be updated but I could already use some feedback.

    Also, I think we should add a try-except in the callback function, especially since users can set their own task factory. For instance:

    loop.set_task_factory(lambda loop, coro: i_raise_an_exception)

    will cause the future returned by run_coroutine_threadsafe to wait forever. Instead, we could have:

        except Exception as exc:
            if future.set_running_or_notify_cancel():
                future.set_exception(exc)

    inside the callback to notify the future.

    @gvanrossum
    Copy link
    Member Author

    The docs look good. (I assume you've generated the HTML and checked that the output looks good, links are clickable etc.) What do you need to add to the concurrency and multithreading section?

    I agree on the try/except; can you add that to the same diff? (Merging diffs into three different branches is a pain so I want to have to do as little as possible of it.)

    @vxgmichel
    Copy link
    Mannequin

    vxgmichel mannequin commented Oct 5, 2015

    The docs look good.

    Should I add a note to explain why the loop argument has to be explicitly passed? (there is a note at the beginning of the task functions section stating "In the functions below, the optional loop argument ...")

    What do you need to add to the concurrency and multithreading section?

    This section provides an example to schedule a coroutine from a different thread using ensure_future and call_soon_threadsafe. This example should be replaced with another usage of call_soon_threadsafe and another paragraph about run_coroutine_threadsafe should be added.

    I agree on the try/except

    Do you think the exception should be re-raised for the logger?

    can you add that to the same diff?

    All right, should I make another PR on the asyncio github repo as well?

    @gvanrossum
    Copy link
    Member Author

    Should I add a note to explain why the loop argument has to be explicitly
    passed? (there is a note at the beginning of the task functions section
    stating "In the functions below, the optional loop argument ...")

    Oh, that's a good idea. It's sort of clear because it starts with "from a
    different thread" but it doesn't hurt to say the same thing in different
    ways, because it's an easy mistake to make.

    > What do you need to add to the concurrency and multithreading section?

    This section provides an example to schedule a coroutine from a different
    thread using ensure_future and call_soon_threadsafe. This example
    should be replaced with another usage of call_soon_threadsafe and another
    paragraph about run_coroutine_threadsafe should be added.

    Oh, yes, definitely update that!

    > I agree on the try/except

    Do you think the exception should be re-raised for the logger?

    Yes.

    > can you add that to the same diff?

    All right, should I make another PR on the asyncio github repo as well?

    No, it's easier for me to copy the changes from CPython into the asyncio
    repo.

    @1st1
    Copy link
    Member

    1st1 commented Oct 5, 2015

    Should I add a note to explain why the loop argument has to be explicitly
    passed? (there is a note at the beginning of the task functions section
    stating "In the functions below, the optional loop argument ...")

    Oh, that's a good idea. It's sort of clear because it starts with "from a
    different thread" but it doesn't hurt to say the same thing in different
    ways, because it's an easy mistake to make.

    Can we make 'loop' a required argument (no default None value)?

    @gvanrossum
    Copy link
    Member Author

    loop *is* required for this function. It's just that there's an earlier, general comment describing it as optional for all functions in this section.

    @vxgmichel
    Copy link
    Mannequin

    vxgmichel mannequin commented Oct 5, 2015

    I attached a patch that should sum up all the points we discussed.

    I replaced the call_soon_threadsafe example with:
    loop.call_soon_threadsafe(callback, *args)
    cause I couldn't find a simple specific usage. Let me know if you think of a better example.

    @gvanrossum
    Copy link
    Member Author

    I think it's fine. I'm going to commit this now. Thanks again!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 5, 2015

    New changeset 54c77fdcdb2e by Guido van Rossum in branch '3.4':
    Docs and one small improvement for issue bpo-25304, by Vincent Michel.
    https://hg.python.org/cpython/rev/54c77fdcdb2e

    New changeset 28fcd7f13613 by Guido van Rossum in branch '3.5':
    Docs and one small improvement for issue bpo-25304, by Vincent Michel. (Merge 3.4->3.5.)
    https://hg.python.org/cpython/rev/28fcd7f13613

    New changeset cba27498a2f7 by Guido van Rossum in branch 'default':
    Docs and one small improvement for issue bpo-25304, by Vincent Michel. (Merge 3.5->3.6.)
    https://hg.python.org/cpython/rev/cba27498a2f7

    @gvanrossum
    Copy link
    Member Author

    Thanks!

    @bitdancer
    Copy link
    Member

    I think the 'versionchanged' should say "3.4.4, 3.5.1". We've already had one user confused by the fact that it isn't in 3.5.0.

    @1st1
    Copy link
    Member

    1st1 commented Nov 18, 2015

    Good catch! Fixed in https://hg.python.org/cpython/rev/b34c42e46e7b

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants