[Python-bugs-list] [ python-Bugs-471720 ] ThreadingMixIn/TCPServer forgets close
noreply@sourceforge.net
noreply@sourceforge.net
Sat, 20 Oct 2001 01:56:30 -0700
Bugs item #471720, was opened at 2001-10-16 07:04
You can respond by visiting:
http://sourceforge.net/tracker/?func=detail&atid=105470&aid=471720&group_id=5470
Category: Python Library
Group: Python 2.2
Status: Closed
Resolution: Accepted
Priority: 5
Submitted By: Max Neunhöffer (neunhoef)
Assigned to: Guido van Rossum (gvanrossum)
Summary: ThreadingMixIn/TCPServer forgets close
Initial Comment:
When using the SocketServer.TCPServer class in connection with the SocketServer.ThreadingMixIn class
every request produces an open socket, because
the overloaded process_request method only calls
the finish_request method without calling close_request afterwards.
The test in test_socketserver.py does not notice
this.
This is true for Python 2.1.1 and Python 2.2a4.
Documentation for ThreadingMixIn (and ForkingMixIn)
is a little meager.
----------------------------------------------------------------------
>Comment By: Max Neunhöffer (neunhoef)
Date: 2001-10-20 01:56
Message:
Logged In: YES
user_id=350896
You are right. In the current version exceptions are not
properly handled: The new thread for a request does not
catch exceptions. However, the new thread should handle the
exception. Therefore:
I suggest to exactly copy the behaviour in "handle_request"
in "process_request_thread" (see patch): All exceptions are
caught, in case of an exception in either finish_request or
close_request the "handle_error" method is called and then
"close_request" is done.
This means, that when "close_request" causes the exception,
it is tried again after "handle_error". Another possibility
would be to move "close_request" out of the "try"-clause...
During testing this I found another problem: In case
of an exception the files "rfile" and "wfile" in the
BaseRequestHandler instance are not closed. Because
they contain dup'ed file descriptors of the socket, the
connection remains open, probably until this instance is
garbage collected.
Therefore I suggest the following to resolve this problem:
Move the call to the "finish" method in "__init__" of
BaseRequestHandler into the "finally" clause. Then the files
are properly closed and the exception is propagated further
up. This is also in the patch.
This however changes the behaviour of this class also in the
non-Threading case from the user's point of view (if the
finish method is overloaded)! I do not know whether you want
to introduce such a change into the code between alpha and
beta releases...
----------------------------------------------------------------------
Comment By: Guido van Rossum (gvanrossum)
Date: 2001-10-18 11:01
Message:
Logged In: YES
user_id=6380
OK. That looks good. I've checked this into CVS, in time for
2.2b1.
Question: what should be done if an exception occurs in
finish_request()?
----------------------------------------------------------------------
Comment By: Max Neunhöffer (neunhoef)
Date: 2001-10-17 02:10
Message:
Logged In: YES
user_id=350896
This is a context diff of a proposal for a patch
relative to the current state of CVS.
I have to admit that I tested this only with 2.2a4
and not with the current CVS version.
----------------------------------------------------------------------
Comment By: Guido van Rossum (gvanrossum)
Date: 2001-10-16 17:26
Message:
Logged In: YES
user_id=6380
OK. It would be a great help if you could upload a patch (a
context diff) relative to the current state of CVS, or the
2.2a4 release if you must. When uploading, don't forget to
check the file upload checkbox.
----------------------------------------------------------------------
Comment By: Max Neunhöffer (neunhoef)
Date: 2001-10-16 15:01
Message:
Logged In: YES
user_id=350896
I suggest changing only the ThreadingMixIn class in the
following way:
(1) Add a method process_request_thread which does exactly
the same as
the process_request method of the BaseServer class:
call finish_request
call close_request
(2) The overloaded process_request method does not launch
the finish_request
method as a new thread but instead the
process_request_thread method.
Another possibility would be to somehow access the
process_request method
of the corresponding base class generically. However I do
not know a way
to access this easily and cleanly.
Still the documentation to ThreadingMixIn and ForkingMixIn
could be improved.
I do not know the sourceforge bug tracker well enough to
merge these "threads"
as montenaro suggests.
----------------------------------------------------------------------
Comment By: Skip Montanaro (montanaro)
Date: 2001-10-16 09:12
Message:
Logged In: YES
user_id=44345
I am getting a sense of deja vu.
Before we march down this path, can we revisit (and perhaps collapse this into) the previous "thread" on this subject:
http://sourceforge.net/tracker/?group_id=5470&atid=105470&func=detail&aid=417845
----------------------------------------------------------------------
Comment By: Guido van Rossum (gvanrossum)
Date: 2001-10-16 08:35
Message:
Logged In: YES
user_id=6380
Can you suggest a fix then?
----------------------------------------------------------------------
Comment By: Max Neunhöffer (neunhoef)
Date: 2001-10-16 07:49
Message:
Logged In: YES
user_id=350896
I would not like this, because then the correct code for
a handler of a ThreadingMixIn/TCPServer would be different from a standard TCPServer.
It is also not very intuitive from a programmers point of view, who only wants to use the server library.
----------------------------------------------------------------------
Comment By: Guido van Rossum (gvanrossum)
Date: 2001-10-16 07:31
Message:
Logged In: YES
user_id=6380
Let's make this a documentation issue. When using threading,
the thread is responsible for closing the socket.
----------------------------------------------------------------------
You can respond by visiting:
http://sourceforge.net/tracker/?func=detail&atid=105470&aid=471720&group_id=5470