[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[R. David Murray]

R. David Murray added the comment:

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 :(

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[Indra Talip]

Indra Talip added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[R. David Murray]

R. David Murray added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[Indra Talip]

Indra Talip added the comment:

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.

--
Added file: http://bugs.python.org/file31003/issue-4079-tests-1.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[R. David Murray]

R. David Murray added the comment:

Now we need a test :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[Indra Talip]

Changes by Indra Talip :


--
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[R. David Murray]

Changes by R. David Murray :


--
stage: test needed -> patch review
status: languishing -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[Indra Talip]

Indra Talip added the comment:

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

--
keywords: +patch
nosy: +italip
versions: +Python 3.4 -Python 2.6
Added file: http://bugs.python.org/file30873/issue-4079-1.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[Facundo Batista]

Facundo Batista  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[R. David Murray]

R. David Murray  added the comment:

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.

--
keywords: +easy
nosy: +r.david.murray
priority:  -> normal
stage:  -> unit test needed
status: open -> languishing

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[John J Lee]

Changes by John J Lee <[EMAIL PROTECTED]>:


--
nosy: +facundobatista

___
Python tracker <[EMAIL PROTECTED]>

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[John J Lee]

John J Lee <[EMAIL PROTECTED]> added the comment:

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?)

--
nosy: +jjlee

___
Python tracker <[EMAIL PROTECTED]>

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[Senthil]

Senthil <[EMAIL PROTECTED]> added the comment:

http://bugs.python.org/issue4079

--
nosy: +orsenthil

___
Python tracker <[EMAIL PROTECTED]>

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue4079] new urllib2.Request 'timeout' attribute needs to have a default

[Sidnei da Silva]

New submission from Sidnei da Silva <[EMAIL PROTECTED]>:

'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__()

--
components: Library (Lib)
messages: 74541
nosy: sidnei
severity: normal
status: open
title: new urllib2.Request 'timeout' attribute needs to have a default
type: behavior
versions: Python 2.6

___
Python tracker <[EMAIL PROTECTED]>

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com