Opened 7 years ago

Last modified 7 years ago

#6768 defect new

Proxy (and reverse proxy) should disconnect when the client disconnects

Reported by: Andy Lutomirski Owned by: Andy Lutomirski
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight, Jeremy Thurgood Branch: branches/proxy-disconnect-6768
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

Various backend services want to know when the browser disconnects. Currently web.proxy keeps backend connections open until the backend is done, which prevents this from working.

Fixing this appears to be five lines of code. Can someone take a look?

Attachments (5)

twisted_proxy_disconnect.patch (677 bytes) - added by Andy Lutomirski 7 years ago.
twisted_proxy_disconnect.2.patch (3.3 KB) - added by Andy Lutomirski 7 years ago.
v2, now with a test case
proxy_cr_fixes.patch (3.3 KB) - added by Andy Lutomirski 7 years ago.
response to comment 5
6768-fixes-v2.patch (2.4 KB) - added by Andy Lutomirski 7 years ago.
Replacement patch (against the branch, r40179)
proxy_cr_fixes_20130304.patch (4.0 KB) - added by Andy Lutomirski 7 years ago.
More proxy fixes

Download all attachments as: .zip

Change History (23)

comment:1 Changed 7 years ago by DefaultCC Plugin

Cc: jknight added

Changed 7 years ago by Andy Lutomirski

Changed 7 years ago by Andy Lutomirski

v2, now with a test case

comment:2 Changed 7 years ago by Andy Lutomirski

Keywords: review added

Please review v2. Here's a description:

When twisted.web.proxy.ProxyClient is waiting for the backend server to complete a request, it's possible from the original request (the one from the browser, for example) to disconnect. If this happens, any response from the backend can't possibly be forwarded, and some backends can avoid expensive processing if they know that the browser has disconnected.

As an example, without this patch, a reverse-proxied SockJS backend using the xhr-streaming protocol will never notice that the browser has navigated away or attempted to close the SockJS link. With this patch, it works correctly.

This patch modifies ProxyClient to disconnect from the backend when the original request fails early.

comment:3 Changed 7 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/proxy-disconnect-6768

(In [40178]) Branching to 'proxy-disconnect-6768'

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

(In [40179]) Apply twisted_proxy_disconnect.2.patch

refs #6768

comment:5 Changed 7 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Andy Lutomirski

Thanks! Some thoughts on the patch:

  1. It's almost always better for protocols to do whatever initialization they can (apart from boring, mostly-side-effect free things like setting attributes on themselves) in connectionMade instead of in __init__. I think the new notifyFinish call in __init__ here would be better done in connectionMade: consider what will happen if the proxy connection is never successfully established (or just not established before the client connection is lost). The proxy will try to lose its connection - a connection it doesn't have, and fail with an AttributeError (because of random implementation details).
  2. It would also be good to test the above case to demonstrate that the fixed version of the code does indeed work after the notifyFinish call is moved to connectionMade.
  3. There is a conditional in fatherFailed - in other words, there are two code paths through this method. The patch only adds a single unit test, though - so only one of these code paths is tested. There should be at least one unit tests for each code path.
  4. Since fatherFailed is just a helper to implement this behavior it's probably a good idea to make it private. This way application developers won't see it and think it's something they might be able to use and we're also free to make whatever changes to it we might want to in a future release without regard for backwards compatibility.
  5. Though fatherFailed should be private, it should also have a docstring. :)
  6. Methods should be separated from each other by two blank lines (instead of just one as is the case for the test method added by this patch).
  7. I think this change merits a description in the changelog - so the patch should include a news fragment.

Thanks again! I've checked your patch into a branch (linked from the top of this page). Please generate the next patch against that branch.

Changed 7 years ago by Andy Lutomirski

Attachment: proxy_cr_fixes.patch added

response to comment 5

comment:6 Changed 7 years ago by Andy Lutomirski

I was unable to reproduce whatever inspired me to add the condition to fatherFailed in the first place, so I removed it.

test_parentDisconnectedEarly failed with addErrback in init. I expected test_parentDisconnectedLate to fail if I removed the condition from fatherFailed, but it didn't. I left the test case in anyway.

I've also added a changelog entry.

I think that this would be improved if it used abortConnection instead of loseConnection, but support for abortConnection is spotty. Is it okay to do something like hasattr(self, 'abortConnection')?

comment:7 Changed 7 years ago by Andy Lutomirski

Keywords: review added

Re-adding the review tag (the instructions for contributors don't say whether I'm supposed to, but I'm guessing that I am).

comment:8 Changed 7 years ago by Andy Lutomirski

Owner: Andy Lutomirski deleted

comment:9 Changed 7 years ago by Jeremy Thurgood

Owner: set to Jeremy Thurgood

comment:10 Changed 7 years ago by Jeremy Thurgood

Cc: Jeremy Thurgood added
Keywords: review removed
Owner: changed from Jeremy Thurgood to Andy Lutomirski

Thanks! Since I haven't looked at this ticket before, my review is for the branch with the new patch applied rather than just the new patch:

  1. Please rename __fatherFailed to _fatherFailed. Name-mangled attributes are problematic and seldom useful.
  1. Test docstrings shouldn't start with "Check that" or similar. Rather use something like "The client should disconnect [...]" instead of "Check that the client disconnects [...]" .
  1. test_parentDisconnectedEarly contains no assertions and creates a client variable that is never referenced. There should be at least one assertion in a test otherwise it isn't testing anything. (This test passes even if the behaviour it claims to be testing is reverted.)
  1. test_parentDisconnectedLate appears to call the code being tested *after* the only assertion in the test. (This test passes even if the behaviour it claims to be testing is reverted.)

I don't have commit access, so I haven't updated the branch. Thanks again!

Changed 7 years ago by Andy Lutomirski

Attachment: 6768-fixes-v2.patch added

Replacement patch (against the branch, r40179)

comment:11 in reply to:  10 Changed 7 years ago by Andy Lutomirski

Keywords: review added
Owner: Andy Lutomirski deleted

Thanks for the review.

Replying to jerith:

  1. Please rename __fatherFailed to _fatherFailed. Name-mangled attributes are problematic and seldom useful.

Done.

  1. Test docstrings shouldn't start with "Check that" or similar. Rather use something like "The client should disconnect [...]" instead of "Check that the client disconnects [...]" .

Fixed.

  1. test_parentDisconnectedEarly contains no assertions and creates a client variable that is never referenced. There should be at least one assertion in a test otherwise it isn't testing anything. (This test passes even if the behaviour it claims to be testing is reverted.)

The test is actually okay, I think -- it's testing that nothing crashes. There isn't any terribly interesting expected behavior here. In theory I could test that the request is *not* sent to the client in the first place, but (a) that's sort of orthogonal to the purpose of this branch and (b) the test infrastructure crashes if I try to test that. Fixing the test infrastructure for this would be nontrivial, so I'll leave it alone.

I did add a comment explaining what was going on.

If you revert the move of addErrback in my patch, this test does indeed fail.

  1. test_parentDisconnectedLate appears to call the code being tested *after* the only assertion in the test. (This test passes even if the behaviour it claims to be testing is reverted.)

This test is thoroughly broken and I can't see any point to it. I removed it entirely.

Thanks!

comment:12 Changed 7 years ago by Hynek Schlawack

(In [41675]) Apply 6768-fixes-v2.patch

refs #6768

comment:13 Changed 7 years ago by Hynek Schlawack

(In [41676]) Add topfile from iproxy_cr_fixes.patch

refs #6768

comment:14 Changed 7 years ago by Hynek Schlawack

Keywords: review removed

Thanks for working on this!

I have applied the latest patch and forced a build: https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/proxy-disconnect-6768

  1. The bots are mostly green except:
    1. If you don’t need client in test_parentDisconnectedEarly, there’s no need to introduce the variable.
    2. In _fatherFailed’s docstring, failure needs a param and a type.
  2. I find the import of main a bit odd, since you only use CONNECTION_LOST anyway, I would suggest to import that.
  3. We don’t “should” in test docstrings. :) Jonathan wrote something about that: https://plus.google.com/+JonathanLange/posts/YA3ThKWhSAj You’re testing for facts, not for hopes: “The client disconnects if the parent request is dropped.”
  4. The news fragment is a bit convoluted, try dropping the “will now”. As the standard jp linked mentions, it should be more like “now closes”
  5. hasattr has some nasty…attributes. :) If you want to go down that road, you would use something like disconnectMethod = getattr(self.transport, 'abortConnection', self.transport.loseConnection)
  6. You’ll have to add a test for both cases of course. :)

Please address those and re-submit for review. Thanks again!

comment:15 Changed 7 years ago by Hynek Schlawack

Owner: set to Andy Lutomirski

Changed 7 years ago by Andy Lutomirski

More proxy fixes

comment:16 Changed 7 years ago by Andy Lutomirski

Keywords: review added
Owner: Andy Lutomirski deleted

All done, although the abortConnection thing is a little bit ugly. :) The test for it is possibly even uglier.

comment:17 Changed 7 years ago by Adi Roiban

Keywords: review removed
Owner: set to Andy Lutomirski

This looks good. For the test I would arrange them in Act/Arrange/Assert block to make it easier to read what is the setup for the test, how is the SUT exercised and how is the final state checked.


Not sure if this text is needed:

We want to abort, too, so that we don't tie up resources on the server we're connecting to.

By reading the code, should be easy to see that it also disconnect the client. Not sure if the "don't tie up resources" reason is the only reason why we disconnect in this way.


Is the 'headers={"accept": "text/html"}' argument relevant for this test? Maybe just leave it out if not.


Are comments, inside comments needed?

    # Monkey patch the transport object to add an abortConnection 
    # function.  (This is too specialized to be worth adding to the 
    # core testing code.) 

Maybe just

    # Monkey patch the transport object to add an abortConnection 
    # function.
    # This is too specialized to be worth adding to the 
    # core testing code.

Instead of aborted[0] = True i would go for aborted.append(True). In this way we can also check that the method is not called multiple times.


main.CONNECTION_LOST was not changed as requested by a previous reviewer. Please try to use

from twisted.internet.main import CONNECTION_LOST

Maybe the test for 'test_parentDisconnected(self):' should also check that _finnished is also set.

Here is a refactor of the code for me is much easier to read and understand. Please not that this code is not tested.. is just how I would like it to be.

def connectClient(self, client):
    """
    Make a connection request for proxy client.
    """
    self.assertForwardsHeaders(
        client, 'GET /foo HTTP/1.0',
        {'connection': 'close', 'accept': 'text/html'})
    self.assertTrue(client.transport.connected)

def test_parentDisconnected(self):
    """
    The client should disconnect if the parent request is dropped.
    """
    request = self.makeRequest('foo')
    client = self.makeProxyClient(request, headers={"accept": "text/html"})
    self.connectClient(client)

    request.processingFailed(main.CONNECTION_LOST)

    self.assertFalse(client.transport.connected)

def patchAbortConnection(self, client):
    """
    Monkey patch the transport object to add an abortConnection 
    function.
    This is too specialized to be worth adding to the 
    core testing code.

    Return a list which will contain True for each time
    abort is called.
    """
    self.assertFalse(hasattr(client.transport, 'abortConnection')) 
    aborted = [] 
    def abortConnection(self): 
        self.loseConnection() 
        aborted.append(True) 
    client.transport.abortConnection = ( 
            types.MethodType(abortConnection, None, type(client.transport))) 
    return aborted

def test_parentDisconnectedAbort(self): 
    """ 
    The client aborts if the parent request is dropped and the client's 
    transport supports abortConnection. 
    """ 
    request = self.makeRequest('foo') 
    client = self.makeProxyClient(request, headers={"accept": "text/html"})
    self.connectClient(client) 
    abort_calls = self.patchAbortConnection(client)

    request.processingFailed(main.CONNECTION_LOST) 

    self.assertFalse(client.transport.connected) 
    self.assertEqual([True], abort_calls) 

def test_parentDisconnectedEarly(self):
    """
    The proxy should survive a parent disconnection before the client
    connects.
    """
    request = self.makeRequest('foo')
    client = self.makeProxyClient(request, headers={"accept": "text/html"})

    request.processingFailed(main.CONNECTION_LOST)

    # Note: this test is a little bit odd.  We're testing that the
    # processingFailed call doesn't raise an exception.

Please let me know what do you think about my comments. I am just starting to do reviews so I might be wrong in many ways .

Thanks!

comment:18 Changed 7 years ago by Tom Prince

It isn't clear to me that abort connection should be called, rather than just loseConnection.

Note: See TracTickets for help on using tickets.