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.

Sign up for GitHub

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

Jump to bottom

gh-112169: Documented getaddrinfo/getnameinfo default loop executor usage and implications. #112191

Merged
merged 4 commits into from
Jun 24, 2024
Merged

gh-112169: Documented getaddrinfo/getnameinfo default loop executor usage and implications. #112191

merged 4 commits into from
Jun 24, 2024

Conversation

alex-kowalczyk
Copy link
Contributor

@alex-kowalczyk alex-kowalczyk commented Nov 17, 2023 •
edited by github-actions bot
Loading

Proposing additional note for asyncio.getaddrinfo/asyncio.getnameinfo, to document they employ the loops's default executor which might manifest in "DNS timeouts" when the executor is saturated.
See also: Guido's remark at #112169 (comment)

📚 Documentation preview 📚: https://cpython-previews--112191.org.readthedocs.build/

@cpython-cla-bot CPython CLA Bot
Copy link

cpython-cla-bot bot commented Nov 17, 2023 •
edited
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app bedevere-app bot added awaiting review docs Documentation in the Doc dir skip news labels Nov 17, 2023
@bedevere-app bedevere-app bot mentioned this pull request Nov 17, 2023
Closed
gvanrossum
gvanrossum approved these changes Nov 20, 2023
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Approved, with one tiny addition of my own: you could possibly just configure a bigger thread pool. I'll wait merging for a bit in case you object to this suggestion.

@gvanrossum
Copy link
Member

Thinking about this some more, I'm curious what exactly you mean by "DNS timeouts". Is this referring to some library calling e.g. getaddrinfo() and reporting an error if it doesn't respond within a specific time interval? It's slightly incorrect to flag that as a "DNS timeout" (since the problem is on the application side, not on the DNS side -- at least from the network's POV). I don't see how a saturated executor could lead to actual DNS timeouts, where DNS itself doesn't respond timely to requests. Maybe the documentation note should be more explicit about what kind of errors can happen as a result? It is indeed the case that calls to await loop.getaddrinfo(...) might not return in a timely fashion.

willingc
willingc reviewed Dec 13, 2023
View reviewed changes
Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Addressing @gvanrossum's comment, my suggestion would be to remove the reference to "DNS timeouts" since higher level networking libraries could treat in different ways.

Doc/library/asyncio-eventloop.rst Outdated Show resolved Hide resolved
@kumaraditya303 @willingc
Update Doc/library/asyncio-eventloop.rst
751c584 
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
@kumaraditya303 kumaraditya303 added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jun 24, 2024
@kumaraditya303 kumaraditya303 enabled auto-merge (squash) June 24, 2024 08:15
@kumaraditya303 kumaraditya303 merged commit fc297b4 into python:main Jun 24, 2024
26 checks passed
@miss-islington-app
Copy link

Thanks @alex-kowalczyk for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 24, 2024
@alex-kowalczyk @gvanrossum
@kumaraditya303 @willingc @miss-islington
pythongh-112169: Documented getaddrinfo/getnameinfo default loop exec…
448b790 
…utor usage and implications. (pythonGH-112191)

(cherry picked from commit fc297b4)

Co-authored-by: Alek Kowalczyk <alek.kowalczyk@gmail.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 24, 2024
@alex-kowalczyk @gvanrossum
@kumaraditya303 @willingc @miss-islington
pythongh-112169: Documented getaddrinfo/getnameinfo default loop exec…
40ad58c 
…utor usage and implications. (pythonGH-112191)

(cherry picked from commit fc297b4)

Co-authored-by: Alek Kowalczyk <alek.kowalczyk@gmail.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
@bedevere-app
Copy link

bedevere-app bot commented Jun 24, 2024

GH-120935 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 24, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jun 24, 2024

GH-120936 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jun 24, 2024
kumaraditya303 added a commit that referenced this pull request Jun 24, 2024
@miss-islington @alex-kowalczyk
@gvanrossum @kumaraditya303 @willingc
[3.12] gh-112169: Documented getaddrinfo/getnameinfo default loop exe…
3afb856 
…cutor usage and implications. (GH-112191) (#120936)

gh-112169: Documented getaddrinfo/getnameinfo default loop executor usage and implications. (GH-112191)
(cherry picked from commit fc297b4)

Co-authored-by: Alek Kowalczyk <alek.kowalczyk@gmail.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
kumaraditya303 added a commit that referenced this pull request Jun 24, 2024
@miss-islington @alex-kowalczyk
@gvanrossum @kumaraditya303 @willingc
[3.13] gh-112169: Documented getaddrinfo/getnameinfo default loop exe…
206028d 
…cutor usage and implications. (GH-112191) (#120935)

gh-112169: Documented getaddrinfo/getnameinfo default loop executor usage and implications. (GH-112191)
(cherry picked from commit fc297b4)

Co-authored-by: Alek Kowalczyk <alek.kowalczyk@gmail.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@alex-kowalczyk @gvanrossum
@kumaraditya303 @willingc @mrahtz
pythongh-112169: Documented getaddrinfo/getnameinfo default loop exec…
f7e0105 
…utor usage and implications. (python#112191)

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@alex-kowalczyk @gvanrossum
@kumaraditya303 @willingc @noahbkim
pythongh-112169: Documented getaddrinfo/getnameinfo default loop exec…
bdad391 
…utor usage and implications. (python#112191)

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@alex-kowalczyk @gvanrossum
@kumaraditya303 @willingc @estyxx
pythongh-112169: Documented getaddrinfo/getnameinfo default loop exec…
f64da2f 
…utor usage and implications. (python#112191)

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willingc willingc willingc left review comments

@gvanrossum gvanrossum gvanrossum approved these changes

@kumaraditya303 kumaraditya303 kumaraditya303 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

Assignees
No one assigned
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.
4 participants
@alex-kowalczyk @gvanrossum @willingc @kumaraditya303