Opened 11 years ago

Last modified 11 years ago

#4901 defect new

RunTimeError from t.w.http.Request when parent request loses connection before proxy request finishes

Reported by: Sigmund Augdal Owned by: RN
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight, RN, Tristan Seligmann Branch:
Author:

Description

The issue can be worked around by replacing t.w.proxy.ProxyClient with the following class:

class FixProxyClient(proxy.ProxyClient):
    def __init__(self, command, rest, version, headers, data, father):
        proxy.ProxyClient.__init__(self, command, rest, version, headers, data, father)
        dfr = father.notifyFinish()
        dfr.addBoth(self.parentRequestFinished)
    def parentRequestFinished(self, arg):
        if not self._finished:
            self._finished = True
            self.transport.loseConnection()

Attachments (1)

issue_4901.patch (2.4 KB) - added by RN 11 years ago.
Patch that makes exception a message.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 11 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 11 years ago by Sigmund Augdal

This happens to me when I run my proxy server in valgrind. As I understand the code the steps to reproduce should be:

  • Set up a ReverseProxyResource forwarding requests for a given url to another http server.
  • Send a request to the ReverseProxyResource. Close the connection for this request before the answer is complete.

comment:3 Changed 11 years ago by RN

Cc: RN added

Hmm, I am having the same issue by just using simple xmlrpc client/server. If the client disconnects incorrectly, I get the traceback in the server.

File
"/usr/local/lib/python2.6/site-packages/Twisted-10.0.0-py2.6-linux-x86_64.egg/twisted/web/xmlrpc.py",
line 289, in handleResponse
    self.factory.parseResponse(contents)
  File
"/usr/local/lib/python2.6/site-packages/Twisted-10.0.0-py2.6-linux-x86_64.egg/twisted/web/xmlrpc.py",
line 323, in parseResponse
    deferred.callback(response)
  File
"/usr/local/lib/python2.6/site-packages/Twisted-10.0.0-py2.6-linux-x86_64.egg/twisted/internet/defer.py",
line 280, in callback
    self._startRunCallbacks(result)
  File
"/usr/local/lib/python2.6/site-packages/Twisted-10.0.0-py2.6-linux-x86_64.egg/twisted/internet/defer.py",
line 354, in _startRunCallbacks
    self._runCallbacks()
--- <exception caught here> ---
  File
"/usr/local/lib/python2.6/site-packages/Twisted-10.0.0-py2.6-linux-x86_64.egg/twisted/internet/defer.py",
line 371, in _runCallbacks
    self.result = callback(self.result, *args, **kw)
  File
"/usr/local/lib/python2.6/site-packages/Twisted-10.0.0-py2.6-linux-x86_64.egg/twisted/web/xmlrpc.py",
line 148, in _cbRender
    request.finish()
  File
"/usr/local/lib/python2.6/site-packages/Twisted-10.0.0-py2.6-linux-x86_64.egg/twisted/web/http.py",
line 900, in finish
    "Request.finish called on a request after its connection was lost; "
exceptions.RuntimeError: Request.finish called on a request after its
connection was lost; use Request.notifyFinish to keep track of this.

Obviously, this is with 10.0 but the same error happens with 10.2 too.

comment:4 Changed 11 years ago by Jean-Paul Calderone

This xmlrpc issue is a problem with different code and merits its own ticket.

Changed 11 years ago by RN

Attachment: issue_4901.patch added

Patch that makes exception a message.

comment:5 Changed 11 years ago by RN

Keywords: review added

As discussed at #twisted, it would be probably better to just make it log the message and not raise an exception. Find attached a patch for the same. It applies to revision 31220 and passes all test. I have also updated one of the tests which was failing due to the change.

Includes topfile too.

comment:6 Changed 11 years ago by Tristan Seligmann

Cc: Tristan Seligmann added
Keywords: review removed
Owner: set to RN
  1. The self.addCleanup call in the test should be located directly after the call to log.addObserver; as it stands, if the test assertion fails, the log observer will not be cleaned up.
  2. The line wrapping in the call to log.msg is a bit weird; I think it would be better to start the string on the second line, the way it used to be formatted in the raise statement.
  3. According to the coding standard, explicit line continuations using backslash should be avoided; use parentheses instead. In other words:
            expected = (
                "Request.finish called on a request after its connection "
                "was lost; use Request.notifyFinish to keep track of this.")
    

Other than that, this looks good; unfortunately, I am not a committer, so I cannot merge this patch. However, having reviewed the patch... it seems like this patch actually belongs on a different ticket; this ticket is about the behaviour of ProxyClient, not the behaviour of Request.finish.

Note: See TracTickets for help on using tickets.