Skip to content

Improve asyncio.loop.call_soon() documentation #20883

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

Merged
merged 15 commits into from
Jun 23, 2020
Merged

Improve asyncio.loop.call_soon() documentation #20883

merged 15 commits into from
Jun 23, 2020

Conversation

akindofyoga
Copy link
Contributor

I was initially confused about the difference between loop.call_soon_threadsafe() and run_coroutine_threadsafe(). I believe this change will make this distinction clearer to people who are looking at asyncio for the first time.

@akindofyoga akindofyoga changed the title Imrpove asyncio Event Loop documentation Improve asyncio Event Loop documentation Jun 15, 2020
Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can definitely see that it may be a bit unclear for anyone unfamiliar with callbacks, I don't think the documentation for loop.call_soon() is the best place to define the term. It's a fairly general computer science term and is used in several areas of Python, so I think it would make more sense to define it in the glossary, and link to it the first time the term is used in loop.call_soon(). You can link to a term in the glossary w/ inline Sphinx markup by using the directive :term:. E.g.

:term:`callback`

Also, the definition "a standard Python function" is not entirely wrong per say, but it would be significantly more descriptive and technically accurate to specify that it's:

"A function which is passed as an argument to be executed at a given point in time. Callbacks can be either synchronously or asynchronously executed, depending on whether they are called before or after the result of the function is available."

This is significantly different from just passing a "Python function", as that can also apply to arguments that are not callbacks such as target for threading.Thread() or func in functools.partial(). Those can be potentially used in the process of implementing a callback, but the argument passed to target or func are not considered callbacks in that context.

Would you be interested in working on this @rogerthat94? If not, I should be able to find the time to do it in the relatively near future.

@1st1 As the primary maintainer of the asyncio docs, do you think something like the above would be a reasonable inclusion?

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@aeros aeros changed the title Improve asyncio Event Loop documentation Improve asyncio.call_soon() documentation Jun 16, 2020
@aeros aeros changed the title Improve asyncio.call_soon() documentation Improve asyncio.loop.call_soon() documentation Jun 16, 2020
@aeros
Copy link
Contributor

aeros commented Jun 16, 2020

I've updated the title to more specifically refer to the section of the asyncio documentation that an improvement is being suggested for.

@akindofyoga
Copy link
Contributor Author

akindofyoga commented Jun 17, 2020

@aeros I would be interested in working on this. However, I think you might be misunderstanding what I was confused about when I first read these documents. It was not the definition of the word "callback." I already knew of a callback as "a function that gets executed after an asynchronous event is finished." However, I don't think understanding what a callback is makes it clear that passing a coroutine to loop.call_soon_threadsafe() will cause an error. What are your thoughts on explicitly stating that loop.call_soon_threadsafe() does not work if it gets passed a coroutine?

Could you give me an example of a Python function that should not be passed to loop.call_soon_threadsafe()? Is it just a matter of not running any blocking code in the function? Or are there functions that don't do any blocking that still should not be run with loop.call_soon_threadsafe()?

@aeros
Copy link
Contributor

aeros commented Jun 18, 2020

However, I don't think understanding what a callback is makes it clear that passing a coroutine to loop.call_soon_threadsafe() will cause an error. What are your thoughts on explicitly stating that loop.call_soon_threadsafe() does not work if it gets passed a coroutine?

I'm personally not convinced that it's worth mentioning in this case, since a callback is not a coroutine. The reason being that when you call a coroutine function, it returns a coroutine object (instead of the result of the function), which itself is not callable. Instead, it has to be awaited or an using a function like asyncio.run() (or alternatives such as loop.run_until_complete()). This results in:

TypeError: 'coroutine' object is not callable

If a user tries to pass the coroutine function (without calling it), they'll receive a warning since the coroutine object created is never awaited:

/usr/lib/python3.8/asyncio/events.py:81: RuntimeWarning: coroutine 'coro' was never awaited

In general in the documentation, we try to avoid explicitly mentioning every non-specific possible error (particularly TypeError) that could occur and everything that the function won't accept as arguments. It tends to add noise, and distracts from the main point without communicating anything of substantial value to the reader. It's similar to why asyncio.create_task() doesn't explicitly mention that passing a future to it will result in an error, because it's documented that it's meant to accept coroutines. If a user attempts to do so, the error message will explicitly tell the user that a coroutine was expected:

TypeError: a coroutine was expected, got <Future pending>

Ultimately, I think passing a coroutine to loop.call_soon()/loop.call_soon_threadsafe() is also one of those errors that will occur only once or twice, and then upon seeing the error message the user will realize what was incorrect. If not, then the user is likely misunderstanding how coroutines or callbacks work, which is why I think the most appropriate enhancement would be to the glossary. Their source of confusion would not be specific to loop.call_soon() itself; thus, it would not make sense to me to include in its documentation.

Also, this applies even more to the low-level parts of the documentation, as they're really not intended to be used by the most users in the first place, so they tend to be more succinct. In practice, loop.call_soon()/loop.call_soon_threadsafe() is used primarily by async libraries and in the internals of asyncio. So I'm rather skeptical that this proposed change would substantially benefit users.

However, that's just my opinion. The final decision to merge or reject would fall upon the primary maintainers of asyncio, Andrew and Yury. I can't say for sure of course, but from working with them in the past, I suspect they would concur with most of the above.

Could you give me an example of a Python function that should not be passed to loop.call_soon_threadsafe()?

AFAIK, you can pass any arbitrary Python subroutine function to loop.call_soon() or anything else that expects a callback, including blocking I/O-bound functions. However, it does ultimately defeat the purpose of using it in the first place, since it would block the event loop.

@aeros
Copy link
Contributor

aeros commented Jun 18, 2020

Regardless of the eventual outcome of the PR, I would like to thank @rogerthat94 for the effort to improve the asyncio docs. It's definitely appreciated either way.

At this point, I'll leave it to @1st1 and/or @asvetlov to resolve the decision.

@akindofyoga
Copy link
Contributor Author

akindofyoga commented Jun 19, 2020

@aeros You said a callback is "A function which is passed as an argument to be executed at a given point in time." I don't think this makes it clear that "a callback is not a coroutine." Say I have a function that updates a dict, but does not return anything. I want this to run on the event loop, because other coroutines that run on the event loop also update this dict. It seems reasonable to implement this using a standard python function or a coroutine. This would be a callback either way. Am I missing something here? FWIW passing an async function as a callback in JavaScript works the same way as passing a standard function.

The documentation for asyncio.create_task() says "Wrap the coro coroutine into a Task and schedule its execution." This makes it clear that this function takes a coroutine and not a future. I'm just advocating for the same level of clarity with loop.call_soon(). "Schedule a callback" does not make it clear that a function defined with async will not work. Linking to a glossary definition that would make it clear that callback cannot be a coroutine seems like a fine solution to me. But this definition would have to make it clear to someone familiar with JavaScript that callback must be a non-async function.

you can pass any arbitrary Python subroutine function to loop.call_soon()

Isn't a callback just "any arbitrary function that you want to run after an asynchronous event occurs?" How does defining callback make it clear that a callback shouldn't be defined with "async def?"

I understand that loop.call_soon_threadsafe() is a low-level function. However, this guide, which seems to be an introduction to asyncio for beginners links to it. Would stating that a callback cannot be a coroutine on that page be reasonable?

@akindofyoga
Copy link
Contributor Author

akindofyoga commented Jun 19, 2020

You're welcome! My hope is to prevent others from getting confused between loop.call_soon_threadsafe() and run_coroutine_threadsafe(), like I was. I have updated my changes so that they don't "add noise, and [distract] from the main point" as much.

I hope these new changes will prevent confusion between loop.call_soon_threadsafe() and run_coroutine_threadsafe() without distracting from the main point.

@aeros
Copy link
Contributor

aeros commented Jun 19, 2020

Thanks for explaining your JavaScript background @rogerthat94. I honestly had not considered the difference in semantics between Python's async functions and JS's. I'm somewhat familiar with JS async as well, so I might be able to help clear up some misunderstandings.

Am I missing something here? FWIW passing an async function as a callback in JavaScript works the same way as passing a standard function.

Therein lies the difference - an "async def" function specifically returns a coroutine object, which has a difference in semantics from a standard "def" function (which of course just returns the result). In JS, the async function will return a Promise that represents the eventual result (which is more equivalent to a Future in Python), but is otherwise interchangeable with a standard function.

The reason for this difference is that in asyncio, we intentionally expose users to the Task-based asynchronous model of programming (similar to C#) rather than the callback model. Most user/high-level APIs in asyncio and async frameworks built ontop of it avoid directly returning futures for that reason. While it's significantly different for those who are more familiar with callbacks, it tends to be much easier to learn for most and avoids the infamous situation of "callback hell" in large codebases that deal with many asynchronous operations.

That being said, if you're looking to still use a callback approach because it's more familiar, I would recommend using futures and looking into their API. Although, in my somewhat biased opinion, I think it's worth learning both models so you can choose which one you prefer.

Alternatively, there are alternative async frameworks such as Twisted, which revolves around using callbacks in Deferred objects.

Linking to a glossary definition that would make it clear that callback cannot be a coroutine seems like a fine solution to me. But this definition would have to make it clear to someone familiar with JavaScript that callback must be a non-async function.

If it were specified in the glossary definition that a callback had to specifically be a subroutine function rather than just saying "function" (which could potentially include coroutine functions, e.g. async def or the deprecated @asyncio.coroutine), would it be more clear? If not, we could also consider using "non-coroutine function".

Isn't a callback just "any arbitrary function that you want to run after an asynchronous event occurs?"

It's more so "any arbitrary subroutine function that you want to run at a later point in time" rather than necessarily after an asynchronous operation occurs. For calling it after an asynchronous is completed, you'd likely want to use Future.add_done_callback(). This method will end up calling loop.call_soon() if the future is finished, but will otherwise be called after the future is completed.

However, this guide, which seems to be an introduction to asyncio for beginners links to it. Would stating that a callback cannot be a coroutine on that page be reasonable?

I would not quite call that section a "beginners" area, that specific section is a bit more intermediate-advanced. The main beginners section is here. That being said, I would definitely approve of linking to the callback definition there as well, which could presumably have "non-coroutine function"/"subroutine function" as part of the definition. I agree that we should aim to be specific about that.

@@ -191,8 +191,8 @@ Scheduling callbacks

.. method:: loop.call_soon(callback, *args, context=None)

Schedule a *callback* to be called with *args* arguments at
the next iteration of the event loop.
Schedule a synchronous *callback* to be called with *args*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think this is a substantial improvement over the previous iteration, I would very much prefer to explain that callbacks are subroutine/non-coroutine functions in the Python glossary for "callback", as it universally applies to anything that uses the term in the python docs (including several other areas in the asyncio docs). So rather than repeating it every time we mention callbacks, I think it makes more sense to clarify it in the glossary definition and link to there.

Copy link
Contributor Author

@akindofyoga akindofyoga Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking the time to write out the detailed explanation above! I really appreciate it and it was very helpful. Indeed, I think specifying subroutine function, rather than just function would fix the ambiguity for me. Would this pull request be a good place for me to add that to the glossary and link it in these docs? Or should I define coroutine in the glossary in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think that "synchronous callback" might be a bit misleading or cause some confusion, as a callback can definitely be asynchronous depending on how it is used, such as with Future.add_done_callback(). The function itself passed as a callback is not a coroutine/"async def" function and might be considered "synchronous" in that sense, but the operation is asynchronous since it is happening out of order from the main "flow" of the program.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking the time to write out the detailed explanation above! I really appreciate it and it was very helpful.

No problem, I'm glad to hear it was helpful. :-)

Indeed, I think specifying subroutine function, rather than just function would fix the ambiguity for me. Would this pull request be a good place for me to add that to the glossary and link it in these docs? Or should I define coroutine in the glossary in a separate PR?

Either way works, but it would probably be easier to do it in the same PR. Just make sure to avoid anything that would require a force-push, since it could overwrite the history of the PR and remove some of the review comments (which loses some context for anyone looking in the future).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I added a glossary definition for callback and referenced it from asyncio-eventloop and asyncio-dev.

@@ -191,8 +191,8 @@ Scheduling callbacks

.. method:: loop.call_soon(callback, *args, context=None)

Schedule a *callback* to be called with *args* arguments at
the next iteration of the event loop.
Schedule the *callback* :term:`callback` to be called with
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for asyncio.create_task() says "Wrap the coro coroutine" instead of "Wrap a coro coroutine."

@aeros
Copy link
Contributor

aeros commented Jun 22, 2020

Closing and re-opening the PR to restart travis-ci.

@aeros aeros closed this Jun 22, 2020
@aeros aeros reopened this Jun 22, 2020
Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for working on the requested changes. Since this is a rather fundamental part of the low-level API docs for asyncio, I'd like to provide some time for @1st1 or @asvetlov to review prior to merging.

Co-authored-by: Yury Selivanov <yury@edgedb.com>
@aeros aeros merged commit a16d697 into python:master Jun 23, 2020
@miss-islington
Copy link
Contributor

Thanks @rogerthat94 for the PR, and @aeros for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 23, 2020
* Add a glossary entry for the term "callback"
* Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section

Co-authored-by: Kyle Stanley <aeros167@gmail.com>
(cherry picked from commit a16d697)

Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
@bedevere-bot
Copy link

GH-21063 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jun 23, 2020
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 23, 2020
* Add a glossary entry for the term "callback"
* Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section

Co-authored-by: Kyle Stanley <aeros167@gmail.com>
(cherry picked from commit a16d697)

Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
@bedevere-bot
Copy link

GH-21064 is a backport of this pull request to the 3.8 branch.

@aeros
Copy link
Contributor

aeros commented Jun 23, 2020

Thanks for the review, Yury.

Backporting to 3.9 and 3.8 so it's in the latest versions of the documentation on docs.python.org.

miss-islington added a commit that referenced this pull request Jun 23, 2020
* Add a glossary entry for the term "callback"
* Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section

Co-authored-by: Kyle Stanley <aeros167@gmail.com>
(cherry picked from commit a16d697)

Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
miss-islington added a commit that referenced this pull request Jun 23, 2020
* Add a glossary entry for the term "callback"
* Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section

Co-authored-by: Kyle Stanley <aeros167@gmail.com>
(cherry picked from commit a16d697)

Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
@akindofyoga akindofyoga deleted the patch-2 branch June 23, 2020 02:31
fasih pushed a commit to fasih/cpython that referenced this pull request Jun 29, 2020
* Add a glossary entry for the term "callback"
* Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section

Co-authored-by: Kyle Stanley <aeros167@gmail.com>
arun-mani-j pushed a commit to arun-mani-j/cpython that referenced this pull request Jul 21, 2020
* Add a glossary entry for the term "callback"
* Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section

Co-authored-by: Kyle Stanley <aeros167@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants