SocketServer and broken ThreadingMixIn - patch for 2.1.1c1

[Stian Soiland]

As known, in Python 2.1 the SocketServer.ThreadingMixIn 
does not work, and thereby SocketServer.ThreadingTCPServer
etc. are broken as well.

The cause was the adding of a close_request()-method in
BaseServer and TCPServer. In TCPServer, the close_request()
is (for 2.1.1c1) called from process_request.

This is all nice and good for normal TCPServers. However, 
when mixing in the ThreadingMixIn, bad things happen.
In vanilla 2.1 release, the connection was closed 
right after starting the thread. This is of course
fixed in 2.1.1c1. 

However, the fix simply removes close_request() 
for threaded servers, as the process_request()
is overridden. The overriden process_request()
simply starts final_request() (doing the actual
work), but does not call close_request() 
afterwards.

This means that there is a difference between
threaded servers and non-threading servers. In
threaded servers, the RequestHandler is actually
the one responsible for closing the connection,
but in non-threaded servers (even forked ones)
the connection is closed by calling close_request().

This is not neccesary.
By simply calling BaseServer.process_request()
instead of final_request, everything is done
nicely, inside the thread, and no differences
between non-threaded and threaded servers.

BaseServer.process_request calls both 
final_request() and close_request()


Could anyone please review this and tell  me
if this should be a part of 2.1.x or not?

I believe it would break more code if we in this
release suddenly demand that all threaded
servers (but none of the others) should close
the connection when appropriate. 

By waiting for 2.2 there would be three different
ways for implementors to use ThreadingMixIn, 
depending on wether they have 2.0, 2.1 or 2.2. 
Why would we want that?


Here's my original patch submitted to SourceForge,
for Python bug #417845

<URL: https://sourceforge.net/tracker/?
 group_id=5470&atid=105470&func=detail&aid=417845 >



  Comment By: Stian Soiland (stain)
  Date: 2001-07-13 14:51

  Message:
  Logged In: YES
  user_id=25921

  I don't think it's a good idea to let closing the request be
  the handler's responsibility. It's better to let the ThreadingMixIn
  do it:

  stain at zoidberg:~$ diff -u SocketServer-cvs-1.24.2.1.py SocketServer.py
  --- SocketServer-cvs-1.24.2.1.py        Fri Jul 13 23:20:26 2001
  +++ SocketServer.py     Fri Jul 13 23:34:02 2001
  @@ -451,8 +451,10 @@
       def process_request(self, request, client_address):
           """Start a new thread to process the request."""
           import threading
  -        t = threading.Thread(target = self.finish_request,
  -                             args = (request, client_address))
  +        # Call the BaseServers process_request to close the
  +        # socket after finally_request.
  +        t = threading.Thread(target = BaseServer.process_request,
  +                             args = (self, request, client_address))
           t.start()

  In my eyes this is the best, making the overriden process_request
  behaving like the original process_request. This would make
  the difference between Threading-servers and other servers
  smaller.

  If it looks bad with BaseServer-references inside
  ThreadingMixIn, what about a method of ThreadingMixIn
  named __process_request with the same code as
  BaseServer.process_request?

  -- 
  Stian Soiland


Guido is appearantly rather busy preparing everything
for the 2.1.1 release, and has no time to review this 
simple patch:

  Comment By: Guido van Rossum (gvanrossum)
  Date: 2001-07-17 07:30

  Message:
  Logged In: YES
  user_id=6380

  I have absolutely no time to review this right now.

  This won't be fixed in 2.1.x; I'll look into making that
  change for 2.2 at some point.


My response:

  Comment By: Stian Soiland (stain)
  Date: 2001-07-18 06:36
  Message:
  Logged In: YES
  user_id=25921

  By waiting for 2.2 for this simple patch even more future
  code would break. The programmer would need to write
  seperate code for 2.0, 2.1 and 2.2. Why should we want
  that?

  It should not take much time reviewing these 6 lines
  of change. Anyone else care to comment?

  (This message quoted in posting on comp.lang.python

-- 
Stian Søiland - Trondheim, Norway - http://stain.portveien.to/

        Ikke skill deg ut fra mengden, og gå ordinært kledd. Det er ikke
          nødvendig å vise frem velstanden. [Europeiske reiseforsikring]