-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-83405: Document None support for HTTPConnection.timeout #17843
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
base: main
Are you sure you want to change the base?
Conversation
`timeout` is passed down to `socket.settimeout()` which supports `None` and puts the socket in blocking mode.
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
I just signed the CLA, does this bot auto-update when it gets approved? I'm unsure on how to add a news reference for this, is anything needed? |
The CLA bot should update automatically. See https://devguide.python.org/committing/#what-s-new-and-news-entries for how to add a NEWS entry, but since this is a docs-only change I don't think you actually need one. |
Every change to Python requires a NEWS entry. Please, add it using the blurb_it Web app or the blurb command-line tool. |
@dfarley1 Would you still be interested in giving the PR a quick update? |
Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com> Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
✅ Deploy Preview for python-cpython-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Thanks for updating, and yep I do believe that the policy is squash after merge. |
operations (like connection attempts) will timeout after that many seconds. | ||
If it is ``None`` then blocking operations will not time out. If it is not given, | ||
the global default timeout setting is used. The optional *source_address* | ||
parameter may be a tuple of a (host, port) to use as the source address the | ||
HTTP connection is made from. The optional *blocksize* parameter sets the buffer | ||
size in bytes for sending a file-like message body. |
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'm personally no expert on the module so I can't quite give a proper review, but here's a few more comments:
Saying that the default timeout is used may give rise to the question of what it is, so if the following is accurate what do you think about including something like '... is used, which can be set by socket.settimeout()', since the actual variable being used (socket._GLOBAL_DEFAULT_TIMEOUT
) isn't exposed?
On the side of formatting, to ease reviewing the changes, I would keep the original paragraph structure as close as possible, such as
operations (like connection attempts) will timeout after that many seconds. | |
If it is ``None`` then blocking operations will not time out. If it is not given, | |
the global default timeout setting is used. The optional *source_address* | |
parameter may be a tuple of a (host, port) to use as the source address the | |
HTTP connection is made from. The optional *blocksize* parameter sets the buffer | |
size in bytes for sending a file-like message body. | |
operations (like connection attempts) will timeout after that many seconds. | |
If it is ``None`` then blocking operations will not time out. If it is not given, | |
the global default timeout setting is used. | |
The optional *source_address* parameter may be a tuple of a (host, port) | |
to use as the source address the HTTP connection is made from. | |
The optional *blocksize* parameter sets the buffer size in bytes for | |
sending a file-like message body. |
If I've done it right, only the addition should be shown after committing the change, which highlights the changes better. I wouldn't worry too much about how the raw paragraph looks as (imo) it's better to save review time than have a perfectly formatted rst file.
Another note is that there's this other PR introducing a similar note, which may somewhat invalidate these changes if it gets merged first. I do prefer this version a bit more as it integrates well into the paragraph, but I'll cross-reference this PR just as a heads up.
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.
Perhaps you meant the setdefaulttimeout module function, rather than settimeout (a method)?
The following commit authors need to sign the Contributor License Agreement: |
timeout
is passed down tosocket.settimeout()
which supportsNone
and puts the socket in blocking mode.I know I'm encountering this in 3.6, I'm willing to bet it goes back further than that.
https://bugs.python.org/issue39224