Author neologix
Recipients kristjan.jonsson, neologix, pitrou, vdjeric
Date 2012-04-14.13:39:41
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1334410782.71.0.832978914667.issue14574@psf.upfronthosting.co.za>
In-reply-to
Content
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 :-)
History
Date User Action Args
2012-04-14 13:39:42neologixsetrecipients: + neologix, pitrou, kristjan.jonsson, vdjeric
2012-04-14 13:39:42neologixsetmessageid: <1334410782.71.0.832978914667.issue14574@psf.upfronthosting.co.za>
2012-04-14 13:39:42neologixlinkissue14574 messages
2012-04-14 13:39:41neologixcreate