[Patches] [ python-Patches-1676823 ] Adding timeout to socket.py and httplib.py

[SourceForge.net]

Patches item #1676823, was opened at 2007-03-08 21:32
Message generated for change (Comment added) made by gbrandl
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1676823&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Library (Lib)
Group: Python 2.6
Status: Open
Resolution: Accepted
Priority: 5
Private: No
Submitted By: Facundo Batista (facundobatista)
Assigned to: Georg Brandl (gbrandl)
Summary: Adding timeout to socket.py and httplib.py

Initial Comment:
This is a subset of patch #723312. Like it, adds a NetworkConnection object to socket.py, and then use it from other modules. 

This NetworkConnection basically does what all the other modules do once and again, so no mistery about it (basically calls getaddrinfo over the received address, and try to open a socket to that address).

In this patch I only use this new NetworkConnection from httplib, whose HTTPConnection class now optionally accepts a timeout.

Beyond the changes in socket.py and httplib.py, this patch also includes changes in the docs and new test cases.

----------------------------------------------------------------------

>Comment By: Georg Brandl (gbrandl)
Date: 2007-03-23 18:33

Message:
Logged In: YES 
user_id=849994
Originator: NO

I'm attaching a reviewed version. Basically only nits and one added XXX
comment about IPv6.
File Added: timeout.diff

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2007-03-23 18:21

Message:
Logged In: YES 
user_id=6380
Originator: NO

Can you check this in yourself?

----------------------------------------------------------------------

Comment By: Facundo Batista (facundobatista)
Date: 2007-03-23 17:51

Message:
Logged In: YES 
user_id=752496
Originator: YES

The patch is updated, reflecting discussion in python-dev and last comment
here.

- Just put a timeout default to None. If None, skip settimeout() call.

- Copy the exceptions behaviour that we have actually in the higher
  level libraries, to be sure we aren't breaking anything.

- The function is now public, named "create_connection".

Tests and docs are updated too.
File Added: timeout.diff

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2007-03-21 17:12

Message:
Logged In: YES 
user_id=6380
Originator: NO

I may have missed some of the discussion, but I don't understand why it is
so important to differentiate between explicit timeout=None and omitting
timeout altogether.  I would favor a version where the timeout defaults to
None, is a simple positional argument to _create_connection(), and the
settimeout() call is skipped when timeout is None.

I also favor making create_connection() public again since it may be
useful for user code or 3rd party libraries that implements some protocol. 
After all the reality is probably that people have copied the exact loop
that's found in httplib and 5 other places into their code anyway, if they
care at all about IPv6 support.

----------------------------------------------------------------------

Comment By: Facundo Batista (facundobatista)
Date: 2007-03-21 16:24

Message:
Logged In: YES 
user_id=752496
Originator: YES

I updated the patch, reflecting discussion on python-dev:

- The function name changed, now it's _create_connection(). Its signature
also changed: now, timeout is mandatorily named.

- HTTPConnection has the posibility to call timeout with a number, and
also with None. In both cases, it update sock.timeout (changing effectively
the default timeout).

Docs and tests cases are also updated (note: now there's no doc in
libsocket.tex, as this function is now private).

File Added: timeout.diff

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2007-03-15 18:53

Message:
Logged In: YES 
user_id=6380
Originator: NO

Georg, can you review this?

----------------------------------------------------------------------

Comment By: Facundo Batista (facundobatista)
Date: 2007-03-15 18:31

Message:
Logged In: YES 
user_id=752496
Originator: YES

I refactored the patch, now it uses function connect() in socket.py as
specified in my last comment here.

Of course, tests and docs are also updated in the patch.
File Added: timeout.diff

----------------------------------------------------------------------

Comment By: Facundo Batista (facundobatista)
Date: 2007-03-13 16:11

Message:
Logged In: YES 
user_id=752496
Originator: YES

In all the other libraries we can store the timeout, and then make a
single call to socket.py. 

There's no "refactored connect_with_timeout" in socket.py. The
functionality that will be present is one that is repeated several times in
the higher level modules. So, there's no need of a "..._with_timeout" name.


I'll work in a function, in socket.py, called "connect", with this
functionality, and the following signature:

  def connect(address, timeout=None):   # being address == (host, port)
     ...


----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2007-03-12 21:36

Message:
Logged In: YES 
user_id=6380
Originator: NO

I like the idea of refactoring refactoring of the connect() loop, however,
I wonder if a class isn't overkill.  The constructor could just store the
timeout parameter instead of the networkconnection object, and the
connect() method could just call the refactored connect_with_timeout
function passing it the host/port pair and the timeout.  But perhaps
there's a use case I have missed that appears in one of the classes you
haven't patched yet but looked at?

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1676823&group_id=5470