Opened 5 years ago

Closed 4 years ago

#4179 regression closed fixed (fixed)

HTTP proxy hanging in Twisted 9.0

Reported by: darkporter Owned by: jml
Priority: high Milestone: Twisted-10.0
Component: web Keywords:
Cc: coolcucumber@… Branch: branches/proxy-headers-4179
(diff, github, buildbot, log)
Author: jml Launchpad Bug:

Description

I have an HTTP proxy that uses twisted.web.proxy, which works fine in Twisted 8.2, but after upgrading to Twisted 9.0 I noticed page loads are hanging. The pages appear to fully load, but the browser spinner just keeps spinning forever.

So I grabbed this trivial proxy example from http://wiki.python.org/moin/Twisted-Examples:

from twisted.web import proxy, http
from twisted.internet import reactor
from twisted.python import log
import sys
log.startLogging(sys.stdout)
 
class ProxyFactory(http.HTTPFactory):
    protocol = proxy.Proxy
 
reactor.listenTCP(8080, ProxyFactory())
reactor.run()

And noticed the same thing. I set my proxy settings in Firefox to localhost:8080, load up a page (any page seems to do it, but daringfireball.net does for sure) and it loads, then I hit reload a couple times, and the refresh just spins forever. I tried that exact same code in 8.2 and 9.0, and it's fine in 8.2 but not in 9.0.

My environment is Python 2.6 on Mac OS X.

Change History (12)

comment:1 Changed 5 years ago by exarkun

This was introduced by the fix for #2677. The request.finish() call doesn't always result in the connection being closed. Twisted Web defaults to persistent connections. However, the response headers that get passed through the proxy might include headers which contradict this (.

I think twisted.web.proxy.Proxy needs to be a little more attentive. Here's one hackish workaround:

  • twisted/web/proxy.py

     
    172172 
    173173    requestFactory = ProxyRequest 
    174174 
     175    def checkPersistence(self, request, version): 
     176        return False 
    175177 
    176178 
     179 
    177180class ReverseProxyRequest(Request): 
    178181    """ 
    179182    Used by ReverseProxy to implement a simple reverse proxy. 

This helps just because mistakenly always making the channel non-persistent has a better failure mode than mistakenly always making it persistent. It's not really a correct solution, though.

I would like to say something about "end-to-end" vs "hop-by-hop" headers, but really it's all quite complicated and I don't remember exactly how it works.

comment:2 Changed 5 years ago by exarkun

  • Type changed from defect to regression

comment:3 Changed 5 years ago by darkporter

I updated my example code to this, but I'm still seeing the same problem:

from twisted.web import proxy, http
from twisted.internet import reactor
from twisted.python import log
import sys
log.startLogging(sys.stdout)

class FixedProxy(proxy.Proxy):
    def checkPersistence(self, request, version): 
     	return False
 
class ProxyFactory(http.HTTPFactory):
    protocol = FixedProxy
 
reactor.listenTCP(8080, ProxyFactory())
reactor.run()

comment:4 Changed 5 years ago by darkporter

Let me know if this updated code works for you, maybe I didn't implement your workaround correctly? I was trying to do it without hacking the Twisted library directly.

comment:5 Changed 5 years ago by exarkun

This example doesn't work for me either. I may have made some mistake when testing the patch from my comment above, I'm not sure.

comment:6 Changed 5 years ago by darkporter

I'm kinda stumped on this one. I thought maybe it's the initial state of persistent = 1 so I tried

class FixedProxy(proxy.Proxy):
    def __init__(self):
        proxy.Proxy.__init__(self)
        self.persistent = 0
    def checkPersistence(self, request, version): 
        print "checkPersistence"
     	return False

But then it doesn't work at all. I guess I need to grok the changes for #2677.

comment:7 Changed 5 years ago by exarkun

  • Priority changed from normal to high

comment:8 Changed 5 years ago by exarkun

  • Milestone set to Twisted-10.0

comment:9 Changed 4 years ago by jml

  • Author set to jml
  • Branch set to branches/proxy-headers-4179

(In [28296]) Branching to 'proxy-headers-4179'

comment:10 Changed 4 years ago by jml

  • Keywords review added; http proxy hanging removed
  • Owner jknight deleted

Jean-Paul, Itamar & I have prepared a patch. It fixes the problem by deleting the keep-alive header. The keep-alive header if included confuses the web server into thinking that the proxy supports keep-alive connections.

comment:11 Changed 4 years ago by therve

  • Keywords review removed
  • Owner set to jml

Just one comment:

+        if 'keep-alive' in headers:
+            del headers['keep-alive']

You can use headers.pop("keep-alive", None) here.

Please merge!

comment:12 Changed 4 years ago by jml

  • Resolution set to fixed
  • Status changed from new to closed

(In [28307]) Merge in proxy-headers-4179

  • Author: jml, itamar, exarkun
  • Reviewer: therve
  • Fixes #4179

Don't pass the keep-alive headers on from the proxy

Note: See TracTickets for help on using tickets.