-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
bpo-34831: Asyncio tutorial #9748
Conversation
9089420
to
a7a9f25
Compare
cc @ambv -- Łukasz, would be great if you could take a look at this too and share some of your thoughts/experience. |
also @elprans please take a look. |
a7a9f25
to
4b47460
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cjrh. A few comments so far.
9f21e81
to
30d158d
Compare
Docs failure could be due to recent logging related changes to fix Sphinx deprecation warnings in ee171a2 |
Upstream issue : https://bugs.python.org/issue35036 Running (venv) ➜ Doc git:(0e82bc95f2) ✗ make suspicious
mkdir -p build
Building NEWS from Misc/NEWS.d with blurb
PATH=./venv/bin:$PATH sphinx-build -b suspicious -d build/doctrees -D latex_elements.papersize= . build/suspicious
Running Sphinx v1.8.1
loading pickled environment... done
loading ignore rules... done, 343 rules loaded
building [mo]: targets for 0 po files that are out of date
building [suspicious]: targets for 484 source files that are out of date
updating environment: [] 0 added, 11 changed, 0 removed
reading sources... [100%] whatsnew/changelog
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [ 30%] library/asyncio-tutorial/case-study-chat-server
/Users/karthikeyansingaravelan/stuff/python/cpython/Doc/venv/lib/python3.7/site-packages/sphinx/application.py:388: RemovedInSphinx20Warning: app.warning() is now deprecated. Use sphinx.util.logging instead.
RemovedInSphinx20Warning)
writing output... [100%] whatsnew/index
WARNING: [library/asyncio-tutorial/case-study-chat-server:49] ":TODO" found in " (ref:TODO) to create a TCP server very"
build finished with problems, 1 warning.
make[1]: *** [build] Error 1
Suspicious check complete; look for any errors in the above output or in build/suspicious/suspicious.csv. If all issues are false positives, append that file to tools/susp-ignored.csv.
make: *** [suspicious] Error 1 |
a6668dd
to
7c95086
Compare
7c95086
to
8c2f3ff
Compare
Hi @cjrh, Thanks for updating the PR. I will do my best to review it this week. Hope all is well in your world. ☀️ |
Yeah, it's a huge improvement; I'll also try to find time to review this before 3.8 final. |
The unreadable build failure is due to Sphinx deprecating a function, I'm currently fixing it, but in the meantime here's the error you have to fix (or add to
|
Thanks for the update, I'll rebase that soon. |
576ef44
to
26cc634
Compare
This is huge, thank you, @cjrh! I'm a bit busy at the sprints right now, I'll review this soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very impressive.
Please use a new stream API instead of legacy reader/writer.
Async Functions, And Other Syntax Features | ||
========================================== | ||
|
||
Regular Python functions are created with the keyword ``def``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regular Python functions are created with the keyword ``def``, | |
Regular Python functions are created with the keyword :keyword:`def`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushing :keyword:
everywhere for building a link doesn't make sense but the first occurrence in a page can be a link, it's convenient maybe.
asyncio.run(f(1, 2)) | ||
|
||
The first difference is that the function declaration is spelled | ||
``async def``. The second difference is that async functions cannot be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
``async def``. The second difference is that async functions cannot be | |
:keyword:`async def`. The second difference is that async functions cannot be |
|
||
The first difference is that the function declaration is spelled | ||
``async def``. The second difference is that async functions cannot be | ||
executed by simply evaluating them. Instead, we use the ``run()`` function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
executed by simply evaluating them. Instead, we use the ``run()`` function | |
executed by simply evaluating them. Instead, we use the :func:`~asyncio.run` function |
The first difference is that the function declaration is spelled | ||
``async def``. The second difference is that async functions cannot be | ||
executed by simply evaluating them. Instead, we use the ``run()`` function | ||
from the ``asyncio`` module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the ``asyncio`` module. | |
from the :mod:`asyncio` module. |
------------------------- | ||
|
||
The ``run`` function is only good for executing an async function | ||
from "synchronous" code; and this is usually only used to execute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from "synchronous" code; and this is usually only used to execute | |
from *synchronous* code; and this is usually only used to execute |
Using A Queue To Move Data Between Long-Lived Tasks | ||
--------------------------------------------------- | ||
|
||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
Best Practices For Timeouts | ||
--------------------------- | ||
|
||
- start with ``asyncio.wait_for()`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- start with ``asyncio.wait_for()`` | |
- start with :func:`asyncio.wait_for()` |
--------------------------- | ||
|
||
- start with ``asyncio.wait_for()`` | ||
- also look at ``asyncio.wait()``, and what to do if not all tasks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- also look at ``asyncio.wait()``, and what to do if not all tasks | |
- also look at :func:`asyncio.wait()`, and what to do if not all tasks |
-------------------------- | ||
|
||
- app shutdown | ||
- how to handle CancelledError and then close sockets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- how to handle CancelledError and then close sockets | |
- how to handle :exc:`~asyncio.CancelledError` and then close sockets |
... | ||
|
||
async def main(): | ||
server = await asyncio.start_server( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New API:
async with asyncio.StreamServer(client_connected_cb, 'localhost', 9011) as server:
await server.serve_forever()
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 suggest splitting this combo PR into smaller sections. |
I am closing this as this will be split into smaller PRs as discussed in the issue. Thanks for the PR! |
https://bugs.python.org/issue34831