Author r.david.murray
Recipients facundobatista, italip, jjlee, ncoghlan, orsenthil, r.david.murray, sidnei
Date 2013-07-25.15:18:10
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1374765491.86.0.490390277862.issue4079@psf.upfronthosting.co.za>
In-reply-to
Content
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.
History
Date User Action Args
2013-07-25 15:18:11r.david.murraysetrecipients: + r.david.murray, facundobatista, jjlee, ncoghlan, orsenthil, sidnei, italip
2013-07-25 15:18:11r.david.murraysetmessageid: <1374765491.86.0.490390277862.issue4079@psf.upfronthosting.co.za>
2013-07-25 15:18:11r.david.murraylinkissue4079 messages
2013-07-25 15:18:10r.david.murraycreate