Title: Improve handling of 'timeout' parameter default in urllib.urlopen
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: Nosy List: eric.smith, federico.reghenzani, mcjeff, orsenthil, pitrou, r.david.murray
Priority: normal Keywords: patch

Created on 2012-03-27 15:02 by r.david.murray, last changed 2012-04-07 11:08 by orsenthil. This issue is now closed.

File name Uploaded Description Edit
urllib_urlopen_default.patch mcjeff, 2012-03-27 17:35 review
OpenDirector.patch federico.reghenzani, 2012-03-27 18:01 patch review
Messages (6)
msg156932 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-03-27 15:02
Currently the prototype uses timeout=socket._GLOBAL_DEFAULT_TIMEOUT, and the docs give the prototype as:

  urlopen(url, data=None[, timeout], *, cafile=None, capath=None)

which is unlike most other Python function prototypes in the docs, and makes no sense from a python syntax point of view.  The current implementation makes it impossible to explicitly request the default timeout unless you look in the code and use the marked-private attribute of the socket module.

I suggest the prototype be changed to either timeout=None with an explanation that that means "use the default socket timeout", or to some public sentinel that can be passed explicitly.  I believe there are a number of other examples in the stdlib of None meaning "use the default", so I favor the former.
msg156945 - (view) Author: Jeff McNeil (mcjeff) * Date: 2012-03-27 17:35
Quick little patch to change the default to None and update the corresponding documentation.
msg156947 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-03-27 17:42
Thanks.  Looks good to me.  I'll give Senthil a little time to comment, in case he disagrees with my proposal, before committing it.

We could conceivably apply this as a bug fix to 3.2, since I don't think there is any backward compatibility issue.
msg156948 - (view) Author: Federico Reghenzani (federico.reghenzani) * Date: 2012-03-27 18:01
I think the same change is needed in function open(url, data=None[, timeout]) of OpenerDirector. I added this other patch.
msg157452 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-04 02:07
Well, it turns out that this design is intentional, because actually using the global socket timeout is deprecated (see issue 2451).  (That is, the non-None default value of the timeout parameter is a backward compatibility hack.)

So I'm closing this as invalid.  Sorry for the confusion, and thanks for the patches, even though they didn't get used this time.  (They did help me realize the issue was invalid though, because the tests didn't pass when I applied them!)

I still don't like the doc artifact, but I can't think of a way to improve it given the nature of the API.
msg157727 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2012-04-07 11:08
Hi David,

I am sorry, I did not notice your second comment in this bug and later when you closed this, noticed the bug report. 

Yes, the default=None but actually pointing to a sentinel value is an odd duck and I believe the explanation in  docs were updated a couple of times to inform user of that behavior, but still the signature gives a feeling that it could be improved. I am at loss as well in terms of giving an "easy solution" to fix the docs.

Date User Action Args
2012-04-07 11:08:07orsenthilsetmessages: + msg157727
2012-04-04 02:07:27r.david.murraysetstatus: open -> closed

nosy: + pitrou
messages: + msg157452

resolution: not a bug
stage: commit review -> resolved
2012-03-27 19:16:18eric.smithsetnosy: + eric.smith
2012-03-27 18:01:22federico.reghenzanisetfiles: + OpenDirector.patch
nosy: + federico.reghenzani
messages: + msg156948

2012-03-27 17:42:21r.david.murraysetmessages: + msg156947
stage: needs patch -> commit review
2012-03-27 17:35:39mcjeffsetfiles: + urllib_urlopen_default.patch

nosy: + mcjeff
messages: + msg156945

keywords: + patch
2012-03-27 15:02:37r.david.murraycreate