bpo-8595: Update urllib, httplib, smtplib docs to warn about global timeout value #27087
Conversation
|
This PR is stale because it has been open for 30 days with no activity. |
|
http.client (also urllib) smtplib |
|
@MaxwellDupre thanks for reviewing, updated per your comments. |
Doc/library/http.client.rst
Outdated
| @@ -46,6 +46,9 @@ The module provides the following classes: | |||
| The optional *blocksize* parameter sets the buffer size in bytes for | |||
| sending a file-like message body. | |||
|
|
|||
| Warning: the global default timeout value is set with no timeout, which may | |||
There was a problem hiding this comment.
As a user my next question would be "how do I set this global timeout?". Is there a place in the docs we can link to that explains how to set it?
Also, style nit: I'd remove the "warning:" part.
There was a problem hiding this comment.
The issue is that the global default timeout is set for sockets, - socket.setdefaulttimeout() - and referencing it would imply a recommendation to use it; but it would not be appropriate for sockets to have the same timeout as for http client. Various ideas like adding a similar default timeout along with get/set functions for httplib were discussed but there wasn't a strong agreement on the best approach, and the last messages were from 12 years ago.
For adding a reference to setdfaulttimeout which notes that default value is to never block, we could potentially also note that it's only for reference, and should not be used for setting the timeout, but it seems a bit awkward to reference something that shouldn't be used.
Another idea is to split off a paragraph in socket.setdefaulttimeout() doc with an anchor so that it only describes the value of timeout, but is not a direct link to the function itself. This is a bit better, but the downside is that function is still right above it; and the current doc works well as a single paragraph, and I think splitting it will make it slightly less readable.
I've removed leading 'Warning: '
There was a problem hiding this comment.
So the intent of this change is so that the user says "I don't like this global default value, and there's no direction on how to change it, so the only option is to set it explicitly" - which is probably the best course of action with current design.
github-actions
bot
removed
the
stale
|
| Name | Link |
|---|---|
| d1d04bf | |
| https://app.netlify.com/sites/python-cpython-preview/deploys/6396a562219b3b00090d67eb |
Doc/library/urllib.request.rst
Outdated
| @@ -657,6 +662,10 @@ OpenerDirector Objects | |||
| timeout setting will be used). The timeout feature actually works only for | |||
| HTTP, HTTPS and FTP connections). | |||
|
|
|||
| .. note:: | |||
|
|
|||
| The global default timeout value is to never time out, which is often | |||
There was a problem hiding this comment.
I believe that this is the original wording and should be updated to the revised wording (as above). Note also this pr which adds a similar note into http.client, so there may be some conflict here if that's merged.
There was a problem hiding this comment.
You are right, thanks for spotting this - I forgot to pull changes when making a previous update.
The purpose of this PR is to point out what the global default timeout is, and that it's often not a good idea to use that value. The #17843 doesn't clearly point out these 2 things so I think if it's merged, it shouldn't have effect on this PR.
https://bugs.python.org/issue8595