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.

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

urlopen() has a hidden default for its timeout argument #62617

Open
warsaw opened this issue Jul 9, 2013 · 6 comments
Open

urlopen() has a hidden default for its timeout argument #62617

warsaw opened this issue Jul 9, 2013 · 6 comments
Labels
stdlib Python modules in the Lib dir

Comments

@warsaw
Copy link
Member

warsaw commented Jul 9, 2013

BPO 18417
Nosy @warsaw, @bitdancer

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2013-07-09.20:11:09.047>
labels = []
title = 'urlopen() has a hidden default for its timeout argument'
updated_at = <Date 2020-08-30.18:37:00.988>
user = 'https://github.com/warsaw'

bugs.python.org fields:

activity = <Date 2020-08-30.18:37:00.988>
actor = 'facundobatista'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2013-07-09.20:11:09.047>
creator = 'barry'
dependencies = []
files = []
hgrepos = []
issue_num = 18417
keywords = []
message_count = 6.0
messages = ['192773', '192774', '192791', '192807', '192809', '192812']
nosy_count = 2.0
nosy_names = ['barry', 'r.david.murray']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue18417'
versions = ['Python 3.4']

@warsaw
Copy link
Member Author

warsaw commented Jul 9, 2013

urllib.request.urlopen() takes a timeout argument with a default value, but the default value is "hidden" and undocumented. As implemented, the value is socket._GLOBAL_DEFAULT_TIMEOUT, but even this isn't really correct or useful.

The problem comes if you are passing a set of arguments to urlopen() and want to pass in a timeout that is the same as default. Because its undocumented, you have to UTSL to figure out what the value is, and then you have to use a non-public attribute of the socket module.

It would be better if urlopen() was documented to default timeout=None meaning "use the default timeout". The implementation should then use socket.getdefaulttimeout() when timeout=None.

The documentation should also be updated.

Now if you want to call urlopen() with the default values, it would just be urlopen(..., timeout=None, ...)

@warsaw
Copy link
Member Author

warsaw commented Jul 9, 2013

Targeting only 3.4 because, while this should no break any (reasonable <wink>) backward compatibility, it is technically an API change and thus should not get backported.

@bitdancer
Copy link
Member

Please see bpo-14425 and then let us know if this is still valid. It's been a while since I looked at the code and I no longer remember the details, but I seemed confident in my conclusion at the time... :)

@bitdancer
Copy link
Member

See also bpo-4079, if you are concerned about timeouts in urllib.

@warsaw
Copy link
Member Author

warsaw commented Jul 10, 2013

On Jul 10, 2013, at 05:53 AM, R. David Murray wrote:

Please see bpo-14425 and then let us know if this is still valid. It's
been a while since I looked at the code and I no longer remember the details,
but I seemed confident in my conclusion at the time... :)

I'm not sure I completely understand the arguments in bpo-14425 and bpo-2451 which
it refers to. I could be wrong, but it seems to me that these are arguing
against using socket.getdefaulttimeout() in favor of the magic non-public
attribute that's currently being used. While I don't like that particular
aspect, it's not the real point of *this* bug.

Even if socket._GLOBAL_DEFAULT_TIMEOUT is the effective default for timeout,
I still think using that as the default value for the keyword argument is
wrong, both for usability of this API and for the documentation of it.

I don't see why timeout=None wouldn't be acceptable, but let's assume it
isn't. A documented, public sentinel would be acceptable and it could either
be defined as:

DEFAULT_TIMEOUT = object()

or

DEFAULT_TIMEOUT = socket._GLOBAL_DEFAULT_TIMEOUT

With the latter, the function's signature would then change to:

def urlopen(url, data=None, timeout=DEFAULT_TIMEOUT, ...)

and nothing functional would change. The documentation update would be much
more helpful too. If it weren't for the new module attribute, it could
probably even be backported.

Then users of this module could do something like this:

from urllib.request import DEFAULT_TIMEOUT, urlopen

def my_open(url, timeout=None):
    my_timeout = DEFAULT_TIMEOUT if timeout is None else timeout
    return urlopen(url, timeout=my_timeout)

@bitdancer
Copy link
Member

OK, I reviewed the issue enough to remember: If socket.setdefaulttimeout is never called, then the default timeout is None (no timeout). Since socket.setdefaulttimeout is deprecated, the global default should always be None. Therefore, if you want to use the "default" timeout in a new application, you could pass None as the value for timeout...but of course that probably doesn't do what you want. What you *really* want to do is pass the default timeout value being used by your application, whatever that is, managed however your application/library manages it.

If you are writing a library that for some reason wants to retain backward compatibility with setdefaulttimeout (and I think you probably shouldn't be), you could call getdefaulttimeout to get the current default value. Since urlopen is creating a new socket object, this should have the correct semantics.

Mind, I don't claim that there can't be a better solution, I'm just trying to explain the current state of affairs as I understand it. However, aside from the ugliness of the current signature for urlopen, at the moment it seems reasonable to me :)

That said, this argument is premised firmly on setdefaulttimeout being deprecated, and there is currently no indication of that in its documentation that I can see.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@iritkatriel iritkatriel added the stdlib Python modules in the Lib dir label Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir
Projects
None yet
Development

No branches or pull requests

3 participants