[issue14574] SocketServer doesn't handle client disconnects properly

[Matej Cepl]

Matej Cepl  added the comment:

The last patch (clear_buffer_on_error.patch) has still not been applied 
(looking at 2.7 and master branches).

--
nosy: +mcepl

___
Python tracker 

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

[issue14574] SocketServer doesn't handle client disconnects properly

[John Szakmeister]

John Szakmeister added the comment:

FWIW, I'm seeing the same issue as Jason too, and his solution seems reasonable 
to me.

--
nosy: +jszakmeister

___
Python tracker 

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Jason Morawski]

Jason Morawski added the comment:

Found another issue with this in 2.7

If the socket.error gets raised from the call to self.wfile.flush(), then when 
self.wfile.close() is called, self.wfile.flush() is called a second time if 
self.wfile._wbuf is not empty. This will raise the socket.error again. 
Attaching a patch that explicitly clears the buffer when the socket.error 
occurs.

--
nosy: +Jason Morawski
Added file: http://bugs.python.org/file40772/clear_buffer_on_error.patch

___
Python tracker 

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Charles-François Natali]

Charles-François Natali added the comment:

 Semantically, but I am told that due to py3k's different file buffering, that 
 those errors don't percolate through. According to Charles-Francois' post 
 from apr, 14th:
 Note that Python 3.3 doesn't have this problem.

It's not affected by the problem, but it's really just accidental, so
it would probably be better to apply this as well to py3k.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

Ok, I'll have a look.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Changes by Kristján Valur Jónsson krist...@ccpgames.com:


--
versions: +Python 3.2, Python 3.3, Python 3.4

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Roundup Robot]

Roundup Robot added the comment:

New changeset 7e5d7ef4634d by Kristján Valur Jónsson in branch '3.2':
Issue #14574: Ignore socket errors raised when flushing a connection on close.
http://hg.python.org/cpython/rev/7e5d7ef4634d

New changeset 7734c3020a47 by Kristján Valur Jónsson in branch '3.3':
Merge with 3.2 :
http://hg.python.org/cpython/rev/7734c3020a47

New changeset 2d1cfbaef9a2 by Kristján Valur Jónsson in branch 'default':
Merge with 3.3
http://hg.python.org/cpython/rev/2d1cfbaef9a2

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

There, applied the same changes to the 3.x branches.
apparently the DOC folder wasn't branched off for 3.3, or something, at least, 
there is no sepatate 3.3 and 'default' version of socketserver.rst.  Or maybe 
it is just Hg that got the better of me.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Andrew Svetlov]

Andrew Svetlov added the comment:

Everything is fine. Close the issue.

--
stage:  - committed/rejected
status: open - closed

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

So, should I commit this?  The change is really trivial.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Charles-François Natali]

Charles-François Natali added the comment:

 So, should I commit this?  The change is really trivial.

LGTM.
This should be applied to 2.7.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Roundup Robot]

Roundup Robot added the comment:

New changeset df51cb946d27 by Kristján Valur Jónsson in branch '2.7':
Issue #14574: Ignore socket errors raised when flushing a connection on close.
http://hg.python.org/cpython/rev/df51cb946d27

--
nosy: +python-dev

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Changes by Kristján Valur Jónsson krist...@ccpgames.com:


--
resolution:  - fixed

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Andrew Svetlov]

Andrew Svetlov added the comment:

Not clean to me: has python3 the same bug?

--
nosy: +asvetlov

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

Semantically, but I am told that due to py3k's different file buffering, that 
those errors don't percolate through. According to Charles-Francois' post from 
apr, 14th:
Note that Python 3.3 doesn't have this problem.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

 although I'd prefer a BSD errno example, such as ECONNRESET, instead of a 
 winsock one
Are you referring to the comment where I mention ECONNABORTED?  That is a 
regular unix error (windows version is WSAECONNABORTED).
This error occurs when the local stack decides to kill the connection, as 
opposed to ECONNRESET which occurs on account of receiving a RST packet.

New patch includes documentation change.

If 2.7 is still in bugfix mode, then this patch could probably be accepted.

--
Added file: http://bugs.python.org/file27432/socketserver.patch

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Charles-François Natali]

Charles-François Natali added the comment:

 Are you referring to the comment where I mention ECONNABORTED?  That is a 
 regular unix error (windows version is WSAECONNABORTED).
 This error occurs when the local stack decides to kill the connection, as 
 opposed to ECONNRESET which occurs on account of receiving a RST packet.

Oops, I misread. ECONNABORTED is fine.

 New patch includes documentation change.

LGTM.

 If 2.7 is still in bugfix mode, then this patch could probably be accepted.

I guess so.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

Clearly, the finish call is intended to be called, and I think the 
documentation is in error.
However, the finish call should also be able to cope with the connection having 
been reset and handle such errors as may occur.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

Please consider the attached patch and see if it solves the issue.

--
keywords: +patch
Added file: http://bugs.python.org/file27414/socketserver.patch

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Charles-François Natali]

Charles-François Natali added the comment:

 Please consider the attached patch and see if it solves the issue.

The patch looks OK (although I'd prefer a BSD errno example, such as
ECONNRESET, instead of a winsock one).

We should also update the documentation that states that in finish()
won't be called if handle() raises an exception.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Mike Putnam]

Changes by Mike Putnam m...@theputnams.net:


--
nosy: +mikeputnam

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Kristján Valur Jónsson krist...@ccpgames.com added the comment:

I think this needs serious consideration.  There needs to be an socket error 
case cleanup path that releases resources but ignores further socket errors.

--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Kristján Valur Jónsson]

Changes by Kristján Valur Jónsson krist...@ccpgames.com:


--
nosy: +kristjan.jonsson

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Antoine Pitrou]

Changes by Antoine Pitrou pit...@free.fr:


--
nosy: +neologix

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Charles-François Natali]

Charles-François Natali neolo...@free.fr added the comment:

Thanks for the report.

Several things are going on here:
1. Even though socketserver's StreamRequestHandler uses unbuffered wfile for 
the socket

class StreamRequestHandler(BaseRequestHandler):
[...]
rbufsize = -1
wbufsize = 0

# A timeout to apply to the request socket, if not None.
timeout = None

# Disable nagle algorithm for this socket, if True.
# Use only when wbufsize != 0, to avoid small packets.
disable_nagle_algorithm = False

def setup(self):
self.connection = self.request
if self.timeout is not None:
self.connection.settimeout(self.timeout)
if self.disable_nagle_algorithm:
self.connection.setsockopt(socket.IPPROTO_TCP,
   socket.TCP_NODELAY, True)
self.rfile = self.connection.makefile('rb', self.rbufsize)
self.wfile = self.connection.makefile('wb', self.wbufsize)


data is internally buffered by socket._fileobject:

def write(self, data):
data = str(data) # XXX Should really reject non-string non-buffers
if not data:
return
self._wbuf.append(data)
self._wbuf_len += len(data)
if (self._wbufsize == 0 or
self._wbufsize == 1 and '\n' in data or
self._wbuf_len = self._wbufsize):
self.flush()


Usually it doesn't turn out to be a problem because if the object is unbuffered 
the buffer is flushed right away, but in this specific case, it's a problem 
because a subsequent call to flush() will try to drain the data buffered 
temporarily, which triggers the second EPIPE from the 
StreamRequestHandler.finish()
Note that Python 3.3 doesn't have this problem.
While this is arguably a bad behavior, I don't feel comfortable changing this 
in 2.7 (either by changing the write() and flush() method or by just checking 
that the _fileobject is indeed buffered before flushing it).
Moreover, this wouldn't solve the problem at hand in case the user chose  to 
use a buffered connection (StreamRequestHandler.wbufsize  0).

2. I think the root cause of the problem is that the handler's finish() method 
is called even when an exception occured during the handler, in which case 
nothing can be assumed about the state of the connection:


class BaseRequestHandler:
[...]
self.setup()
try:
self.handle()
finally:
self.finish()


Which is funny, because it doesn't match the documentation:

.. method:: RequestHandler.finish()

   Called after the :meth:`handle` method to perform any clean-up actions
   required.  The default implementation does nothing.  If :meth:`setup` or
   :meth:`handle` raise an exception, this function will not be called.


So the obvious solution would be to change the code to match the documentation, 
and not call finish when an exception was raised.
But that would be a behavior change, and could introduce resource leaks.
For example, here's StreamRequestHandler finish() method:

def finish(self):
if not self.wfile.closed:
self.wfile.flush()
self.wfile.close()
self.rfile.close()

While in this specific case if wouldn't lead to a FD leak (because the 
underlying socket is closed by the server code), one could imagine a case where 
it could have a negative impact, so I'm not sure about changing this.

Finally, you could get rid of this error by overriding 
StreamRequestHandler.finish() method or catching the first exception in the 
handle() method and close the connection explicitely.

So I'd like to know what others think about this :-)

--
nosy: +pitrou

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Vladan Djeric]

Vladan Djeric vladandje...@gmail.com added the comment:

Thank you for taking a look Charles-François.

I should note that catching the first exception in the request handler and then 
calling self.wfile.close() wouldn't fully solve the issue. The 
self.wfile.close() call would throw another broken pipe exception (which is ok 
 can be caught), but the BaseHTTPServer code would also throw an exception 
when it tries to flush the (now closed) wfile after returing from the do_POST 
request handler.


Exception happened during processing of request from ('127.0.0.1', 50611)
Traceback (most recent call last):
  File C:\Python27\lib\SocketServer.py, line 284, in _handle_request_noblock
self.process_request(request, client_address)
  File C:\Python27\lib\SocketServer.py, line 310, in process_request
self.finish_request(request, client_address)
  File C:\Python27\lib\SocketServer.py, line 323, in finish_request
self.RequestHandlerClass(request, client_address, self)
  File C:\Python27\lib\SocketServer.py, line 638, in __init__
self.handle()
  File C:\Python27\lib\BaseHTTPServer.py, line 340, in handle
self.handle_one_request()
  File C:\Python27\lib\BaseHTTPServer.py, line 329, in handle_one_request
self.wfile.flush() #actually send the response if not already done.
  File C:\Python27\lib\socket.py, line 303, in flush
self._sock.sendall(view[write_offset:write_offset+buffer_size])
AttributeError: 'NoneType' object has no attribute 'sendall'


--

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

[issue14574] SocketServer doesn't handle client disconnects properly

[Vlad]

New submission from Vlad vladandje...@gmail.com:

When dealing with a new connection, SocketServer.BaseRequestHandler.__init__ 
first calls the request handler (self.handle below) and then calls cleanup code 
which closes the connection (self.finish below).

class BaseRequestHandler:
def __init__(self, request, client_address, server):
 ... snip ... 
try:
self.handle()
finally:
self.finish()

The issue arises when a client disconnects suddenly during the self.handle() 
call. The handler may attempt to write data to the disconnected socket.  This 
will cause an exception (which is correct), but somehow data will still be 
added to the connection's buffer and self.wfile.closed will be False! As a 
result, BaseRequestHandler.finish() will attempt to flush the connection's 
buffer and this will raise another exception which can't be handled without 
modifying the library code.


Exception happened during processing of request from ('127.0.0.1', 62718)
Traceback (most recent call last):
  File C:\Python27\lib\SocketServer.py, line 284, in _handle_request_noblock
self.process_request(request, client_address)
  File C:\Python27\lib\SocketServer.py, line 310, in process_request
self.finish_request(request, client_address)
  File C:\Python27\lib\SocketServer.py, line 323, in finish_request
self.RequestHandlerClass(request, client_address, self)
  File C:\Python27\lib\SocketServer.py, line 641, in __init__
self.finish()
  File C:\Python27\lib\SocketServer.py, line 694, in finish
self.wfile.flush()
  File C:\Python27\lib\socket.py, line 303, in flush
self._sock.sendall(view[write_offset:write_offset+buffer_size])
error: [Errno 10053] An established connection was aborted by the software in 
your host machine


I've provided a toy server below, you can reproduce the issue by submitting a 
request to it with curl and then immediately killing curl:

 curl -d test  http://127.0.0.1:8000/

Toy server code:
===

import BaseHTTPServer
import SocketServer
import time

class ThreadedHTTPServer(BaseHTTPServer.HTTPServer):
  pass

class RequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
  def do_POST(self):
try:
  length = int(self.headers[Content-Length])
  request = self.rfile.read(length)

  print Sleeping. Kill the 'curl' command now.
  time.sleep(10)
  print Woke up. You should see a stack trace from the problematic 
exception below.

  print Received POST:  + request
  self.send_response(200) # --- This somehow adds to the connection's 
buffer!
  self.end_headers()
except Exception as e:
  print Exception:  + str(e)  # - This exception is expected

httpd = ThreadedHTTPServer((127.0.0.1, 8000), RequestHandler)
httpd.serve_forever()
httpd.server_close()

--
components: Library (Lib)
messages: 158240
nosy: vdjeric
priority: normal
severity: normal
status: open
title: SocketServer doesn't handle client disconnects properly
versions: Python 2.7

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