classification
Title: new urllib2.Request 'timeout' attribute needs to have a default
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: facundobatista, italip, jjlee, ncoghlan, orsenthil, r.david.murray, sidnei
Priority: normal Keywords: easy, patch

Created on 2008-10-08 18:29 by sidnei, last changed 2013-07-29 01:40 by r.david.murray.

Files
File name Uploaded Description Edit
issue-4079-1.patch italip, 2013-07-09 04:10 review
issue-4079-tests-1.patch italip, 2013-07-22 10:33 review
Messages (11)
msg74541 - (view) Author: Sidnei da Silva (sidnei) Date: 2008-10-08 18:29
'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__()
msg75776 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2008-11-12 06:28
http://bugs.python.org/issue4079
msg76776 - (view) Author: John J Lee (jjlee) Date: 2008-12-02 20:25
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?)
msg103201 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-15 12:32
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.
msg103734 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2010-04-20 16:37
I'm ok with the proposed changes from Sidnei (yes, a patch is needed).
msg192720 - (view) Author: Indra Talip (italip) * Date: 2013-07-09 04:10
patch initializes the 'timeout' attribute on urllib2.Request.__init__()
msg192833 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-10 20:26
Now we need a test :)
msg193525 - (view) Author: Indra Talip (italip) * Date: 2013-07-22 10:33
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.
msg193697 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-25 15:18
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.
msg193756 - (view) Author: Indra Talip (italip) * Date: 2013-07-26 22:10
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.
msg193846 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-29 01:40
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 :(
History
Date User Action Args
2013-07-29 01:40:54r.david.murraysetmessages: + msg193846
2013-07-26 22:10:09italipsetmessages: + msg193756
2013-07-25 15:18:11r.david.murraysetmessages: + msg193697
2013-07-22 10:33:59italipsetfiles: + issue-4079-tests-1.patch

messages: + msg193525
2013-07-10 20:26:45r.david.murraysetmessages: + msg192833
2013-07-10 06:34:19italipsetnosy: + ncoghlan
2013-07-09 13:07:04r.david.murraysetstatus: languishing -> open
stage: test needed -> patch review
2013-07-09 04:10:12italipsetfiles: + issue-4079-1.patch
versions: + Python 3.4, - Python 2.6
nosy: + italip

messages: + msg192720

keywords: + patch
2010-04-20 16:37:41facundobatistasetmessages: + msg103734
2010-04-15 12:32:40r.david.murraysetstatus: open -> languishing
priority: normal


keywords: + easy
nosy: + r.david.murray
messages: + msg103201
stage: test needed
2008-12-02 20:29:11jjleesetnosy: + facundobatista
2008-12-02 20:25:33jjleesetnosy: + jjlee
messages: + msg76776
2008-11-12 06:28:50orsenthilsetnosy: + orsenthil
messages: + msg75776
2008-10-08 18:29:58sidneicreate