GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-65046: Fix docs about logging in `asyncio` by kumaraditya303 · Pull Request #97559 · python/cpython

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Sep 26, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: asyncio docs should call out that network logging is a no-no #65046

Closes #65046

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@kumaraditya303
fix docs
a97bcaf
@kumaraditya303
kumaraditya303 requested review from 1st1, asvetlov and gvanrossum as code owners September 26, 2022 10:16
@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Sep 26, 2022
@kumaraditya303 kumaraditya303 added topic-asyncio needs backport to 3.10 needs backport to 3.11 only security fixes labels Sep 26, 2022
@kumaraditya303 kumaraditya303 mentioned this pull request Sep 26, 2022
asyncio docs should call out that network logging is a no-no #65046
Closed
@kumaraditya303 kumaraditya303 assigned gvanrossum Sep 26, 2022
merwok
merwok reviewed Sep 26, 2022
View reviewed changes
Comment thread Doc/library/asyncio-dev.rst Outdated


Network logging can block the event loop, it is recommended to use
a separate thread for handling logs or use non blocking io.

@merwok merwok Sep 26, 2022

Copy link
Copy Markdown
Member

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.

I think it should be spelled non-blocking I/O, but the bigger issue is that it’s not immediately clear what one should use: after all, we are in the async I/O docs! Is there a section in the os module or another part of the stdlib that explains how to do non-asyncio non-blocking I/O?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
kumaraditya303
kumaraditya303 commented Sep 26, 2022
View reviewed changes
Comment thread Doc/library/asyncio-dev.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303
Update Doc/library/asyncio-dev.rst
9eb27f0
gvanrossum
gvanrossum approved these changes Sep 26, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

One nit, I don't think a comma is quite right here. I'll apply my suggestion and merge your PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/asyncio-dev.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Sep 26, 2022
gvanrossum and others added 2 commits September 26, 2022 15:19
@gvanrossum
Fix comma
6e53f8b
@gvanrossum
Merge branch 'main' into asyncio-docs
eac2c92
@gvanrossum
gvanrossum merged commit d68c37c into python:main Sep 26, 2022
@miss-islington

miss-islington commented Sep 26, 2022

Copy link
Copy Markdown
Contributor

Thanks @kumaraditya303 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting merge label Sep 26, 2022
@miss-islington

miss-islington commented Sep 26, 2022

Copy link
Copy Markdown
Contributor

Sorry @kumaraditya303 and @gvanrossum, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker d68c37c0d08542a346a13b150a204208bb0408f5 3.11

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Sep 26, 2022
@bedevere-bot

bedevere-bot commented Sep 26, 2022

Copy link
Copy Markdown

GH-97581 is a backport of this pull request to the 3.10 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 26, 2022
@kumaraditya303 @miss-islington
pythonGH-65046: Fix docs about logging in asyncio (pythonGH-97559)
d6efec4
Explain that logging should not use network I/O.
(cherry picked from commit d68c37c)

Co-authored-by: Kumar Aditya <[email protected]>
@merwok

merwok commented Sep 26, 2022

Copy link
Copy Markdown
Member

No reaction to my remark about the vagueness of the new recommendation?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Sep 26, 2022

Copy link
Copy Markdown
Member

No reaction to my remark about the vagueness of the new recommendation?

I apologize, I was in "get things merged" mode and missed that.

That said, I doubt we have something more specific than "use threads or write to disk" -- and while writing to disk is technically not perfect, it's generally considered good enough for asyncio (we've been told that there are async APIs for writing to disk on some platforms, and IIRC there are 3rd party packages that wrap them, but the benefits are at best marginal, and the code isn't portable.

Also, I think the warning is about configuring a logger that uses network I/O rather than about how user code should do their own logging. The point is that asyncio itself sometimes calls the logging module and the user should avoid messing that up by accidentally configuring a network-using logger.

User code can just use the run_in_executor API to do the logging from a thread, but we don't want to do that for asyncio's own logging.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum gvanrossum removed the needs backport to 3.11 only security fixes label Sep 26, 2022
@gvanrossum gvanrossum added the needs backport to 3.11 only security fixes label Sep 26, 2022
@miss-islington

miss-islington commented Sep 26, 2022

Copy link
Copy Markdown
Contributor

Thanks @kumaraditya303 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Sep 26, 2022

Copy link
Copy Markdown

GH-97582 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 26, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 26, 2022
@kumaraditya303 @miss-islington
pythonGH-65046: Fix docs about logging in asyncio (pythonGH-97559)
1d5ca2a
Explain that logging should not use network I/O.
(cherry picked from commit d68c37c)

Co-authored-by: Kumar Aditya <[email protected]>
miss-islington added a commit that referenced this pull request Sep 26, 2022
@miss-islington @kumaraditya303
GH-65046: Fix docs about logging in asyncio (GH-97559)
72a7815
Explain that logging should not use network I/O.
(cherry picked from commit d68c37c)

Co-authored-by: Kumar Aditya <[email protected]>
miss-islington added a commit that referenced this pull request Sep 26, 2022
@miss-islington @kumaraditya303
GH-65046: Fix docs about logging in asyncio (GH-97559)
82932b3
Explain that logging should not use network I/O.
(cherry picked from commit d68c37c)

Co-authored-by: Kumar Aditya <[email protected]>
@kumaraditya303
kumaraditya303 deleted the asyncio-docs branch September 27, 2022 10:50
pablogsal pushed a commit that referenced this pull request Oct 22, 2022
@miss-islington @kumaraditya303 @pablogsal
GH-65046: Fix docs about logging in asyncio (GH-97559)
e65e6de
Explain that logging should not use network I/O.
(cherry picked from commit d68c37c)

Co-authored-by: Kumar Aditya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@gvanrossum gvanrossum gvanrossum approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gvanrossum gvanrossum

Labels

docs Documentation in the Doc dir skip news topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

asyncio docs should call out that network logging is a no-no

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@kumaraditya303 @miss-islington @bedevere-bot @merwok @gvanrossum
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.