'urllib2' has introduced a configurable 'timeout' setting by assigning
to the 'timeout' attribute of the urllib2.Request object. However the
implementation is flawed:

- the 'timeout' attribute is set in OpenerDirector.open() and nowhere else

- if someone overrides OpenerDirector.open() (btw: mechanize does
  this), then the 'timeout' attribute will never be set, breaking
  other parts of the code which require the 'timeout' attribute to be
  present.

A simple workaround for this would be to do one or more of:

a) define the 'timeout' attribute as socket._GLOBAL_DEFAULT_TIMEOUT at
   class-level

b) initialize the 'timeout' attribute on urllib2.Request.__init__()

http://bugs.python.org/issue4079

This bug was known before the release -- unfortunately the original
author of the patch didn't fix them in time.  This and some other
unresolved issues listed are listed on #2451.  "Somebody" should raise
bugs about the rest of them too, and fix them.

(Senthil, is that the bug you meant to refer to?  You pasted this bug's
own URL here, presumably you meant to paste the URL for a different bug?)

Senthil, Facundo, is there a reason this bug shouldn't be fixed, or are we just waiting for someone to come up with a patch?  I'm assuming the latter and setting it to languishing, but maybe this will wake somebody up who is interested in fixing it.

I'm ok with the proposed changes from Sidnei (yes, a patch is needed).

patch initializes the 'timeout' attribute on urllib2.Request.__init__()

Now we need a test :)

patch adds regressions tests to ensure timeout value from OpenerDirector is honoured and a failing test for a default value for Request.timeout that passes when the patch that initialises Request.timeout is applied.

Thanks, but the new test doesn't fail.  With your test patch, test_default_values fails, but that is explicitly checking for the 'timeout' attribute, which isn't actually part of the public API.  The new test passes even without the fix.

Reading the original issue, it seems like the problem is a bit more subtle.  To quote that issue:

 * API weirdness: Only some, special, urllib2.Request objects may be
passed to handlers, because the constructor does not create objects with
a .timeout.

The original report here says the problem occurs if a subclass overrides 'open' (like Mechanize does).  So that problem will only occur if a request gets passed to a handler without having passed through 'open'.  But there is a question whether there is a way for the code that assumes the timeout attribute exists to fail that doesn't involve an override of 'open'.  The answer may be no, but I haven't traced the logic or reviewed the API, so I don't know.

However, it is clear that if open is overridden, and the code doing so doesn't care about timeouts and so doesn't do the attribute set itself, subsequent code will sometimes fail because it assumes that the attribute does exist.  We could write a test that does that kind of override on 'open', and causes one of those code paths that assumes timeout exists to be called.

But...I have some question, now, just how real an issue this is...if you override open, you really should be either calling it via super or replicating its functionality...so if there is not in fact any way to cause the lack of a timeout attribute to cause a failure without overriding open, there may not be a "real" issue here.  Presumably mechanize ran into it because a keyword argument (new feature) was added.  So the mechanize bug was really that there was a backward compatibility issue between 2.6 and previous versions.  Is that still an issue for 2.7?  Perhaps: there could still be code someone will forward port to 2.7 that overrides open and doesn't know about timeout.  So it is worth fixing, since the fix is simple.

Ah sorry you are right the tests didn't test the specific case of someone overriding the OpenerDirector. The intent was to demonstrate that adding a default timeout to Request didn't break things.

I think you are right about how much of an issue this is. I agree that if you are overriding open you probably should be replicating it's functionality and doing something sane about the timeout parameter.

Reading the code and the online docs I think that the more pertinent issue here is that it isn't documented how the timeout is passed to the handlers. That is if someone implements a new handler that could timeout just from reading the documentation it isn't clear how the timeout is passed to the handler. Given that open adds the timeout to the Request object perhaps the issue should be that the timeout attribute should be added to the public api of the Request and the docs modified to suit, thus making it explicit how handlers get the expected timeout. That does uglify the api though as you would have two places where you could set the timeout (on the Request and via open) and currently calling open with a Request, with a non-default timeout, means that open would override the timeout on the Request with whatever was set on the call to open.

So overall I'm fairly ambivalent about how much of an issue the original report was. I think the larger issue is that how timeouts are handled/passed to handlers/processors should probably be documented.

Yes, this is a very good point.  (And your passing test is worthwhile, you are correct.)  

People are expected to be able to write handlers, so clearly the timeout API needs to be documented, if for no other reason than to keep a handler writer from stomping on the timeout attribute inappropriately.  

But there is a code smell to this "API", and I wonder if it is worth someone taking time to think it all through again to see if there is a better way to do it :(

urllib2 became urllib.request in 3.x.  In 2.6, 'timeout' became a parameter of both urlopen and OpenerDirector.open.  In both cases the default was and is the 'global default timeout setting'.  So 'timeout' has a default.

Both functions take a Request object in lieu of a url.  I see no indication that the Request object itself ever has a timeout attribute, at least not in .__init__.  It certainly does not now.  It seems that the idea was that timeouts are a property of an open action, not of the reusable Request object that wraps a url.


CacheFTPHandler.setTimeout() is for FTP handlers.

So it seems that this should be closed as either 'not a bug' or 'out of date'.