Opened 4 years ago

Closed 3 years ago

#9410 defect closed fixed (fixed)

web http/wsgi doesn't fail gracefully when the client closes the connection

Reported by: Corentin Chary Owned by: jswitzer
Priority: highest Milestone:
Component: web Keywords:
Cc: Branch:
Author:

Description

Assume a web view that does "sleep(10)" and a curl that you interrupt before the web page has been rendered, you'll get the following:

2018-04-03 17:37:43+0200 [-] WSGI application error
        Traceback (most recent call last):
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/python/threadpool.py", line 266, in <lambda>
            inContext.theWork = lambda: context.call(ctx, func, *args, **kw)
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/python/context.py", line 122, in callWithContext
            return self.currentContext().callWithContext(ctx, func, *args, **kw)
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/python/context.py", line 87, in callWithContext
            self.contexts.pop()
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/web/wsgi.py", line 522, in run
            self.started = True
        --- <exception caught here> ---
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/web/wsgi.py", line 500, in run
            self.write(elem)
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/web/wsgi.py", line 455, in write
            self.reactor, wsgiWrite, self.started)
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/internet/threads.py", line 122, in blockingCallFromThread
            result.raiseException()
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/python/failure.py", line 385, in raiseException
            raise self.value.with_traceback(self.tb)
        builtins.AttributeError: 'NoneType' object has no attribute 'writeHeaders'

2018-04-03 17:37:43+0200 [-] Unhandled Error
        Traceback (most recent call last):
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/flask_twisted/__init__.py", line 68, in run
            self.run_simple(app, host, port, **options)
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/flask_twisted/__init__.py", line 46, in run_simple
            reactor.run()
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/internet/base.py", line 1243, in run
            self.mainLoop()
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/internet/base.py", line 1252, in mainLoop
            self.runUntilCurrent()
        --- <exception caught here> ---
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/internet/base.py", line 851, in runUntilCurrent
            f(*a, **kw)
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/web/wsgi.py", line 510, in wsgiError
            self.request.loseConnection()
          File "/home/cchary/dev/jiralerts/venv/lib/python3.6/site-packages/twisted/web/http.py", line 1474, in loseConnection
            self.channel.loseConnection()
        builtins.AttributeError: 'NoneType' object has no attribute 'loseConnection'

Looking at the code, the reason is obvious:

    def connectionLost(self, reason):
        """
        There is no longer a connection for this request to respond over.
        Clean up anything which can't be useful anymore.
        """
        self._disconnected = True
        self.channel = None
        if self.content is not None:
            self.content.close()
        for d in self.notifications:
            d.errback(reason)
        self.notifications = []


    def loseConnection(self):
        """
        Pass the loseConnection through to the underlying channel.
        """
        self.channel.loseConnection()

connectionLost() will set self.channel to None, and loseConnection will still try to use the channel (I guess it should check if the channel is None or not).

Change History (22)

comment:1 Changed 3 years ago by Glyph

Looks like Buildbot has hit this as well: https://github.com/buildbot/buildbot/issues/4045

comment:2 Changed 3 years ago by Glyph

Priority: normalhighest

I am not marking this as a regression, because it's in the last release, but we've been seeing this traceback at work periodically with the 18.7 and did not see it previously, so I am bumping the priority (as if anyone pays attention to that) because I do think that this regressed, although I'm not sure exactly when.

comment:3 Changed 3 years ago by Glyph

Looks like our buildbot is hitting this, with some regularity.

comment:5 Changed 3 years ago by Marcin Jedynski

Any news on resolution of this ?

comment:6 in reply to:  5 ; Changed 3 years ago by Glyph

Replying to Marcin Jedynski:

Any news on resolution of this ?

None yet, although I'd certainly love it if there were.

Would you have any interest in submitting a patch? :)

comment:7 in reply to:  6 Changed 3 years ago by Marcin Jedynski

Replying to Glyph:

Replying to Marcin Jedynski:

Any news on resolution of this ?

None yet, although I'd certainly love it if there were.

Would you have any interest in submitting a patch? :)

Well I'm going through my last month of university, after that I would love to get involved in some open source projects so who knows :)

comment:8 Changed 3 years ago by Marcin Jedynski

For sure not within a month from now :)

comment:9 Changed 3 years ago by jswitzer

I'm seeing a similar traceback after upgrading from 15.4 to the latest twisted.

Jan  8 10:24:15 svc python: File "/root/.pyenv/versions/http_configurator/lib/python2.7/site-packages/twisted/web/server.py", line 238, in write
Jan  8 10:24:15 svc python: http.Request.write(self, data)
Jan  8 10:24:15 svc python: File "/root/.pyenv/versions/http_configurator/lib/python2.7/site-packages/twisted/web/http.py", line 1139, in write
Jan  8 10:24:15 svc python: self.channel.write(data)
Jan  8 10:24:15 svc python: exceptions.AttributeError: 'NoneType' object has no attribute 'write'

I see a bunch of queueing-related code was removed at some point.

I'd hazard a guess at the change (I am not familiar with this code - so please take this with a grain of salt - I haven't had time to fully validate this hypothesis):

  • the old behavior was that writing a response to a request with a client that had vanished was OK as long as it was queued (the response going to a string buffer somewhere). Later, when request.finish() was called we'd see the familiar "Request.finish called on a request after its connection was lost" runtime error. This was because self.transport was set to this StringTransport object.
  • the new behaviour is the Attribute error on self.channel (since self.transport is set to self.channel.transport and self.channel has been deleted).

Some fragments of the diff of src/twisted/web/http.py between trunk and 15.4 that led me to this conclusion:

@@ -584,85 +713,33 @@                                                                                                                                                                                           
     content = None                                                                                                                                                                                             
     _forceSSL = 0                                                                                                                                                                                              
     _disconnected = False                                                                                                                                                                                      
+    _log = Logger()                                                                                                                                                                                            
                                                                                                                                                                                                                
-    def __init__(self, channel, queued):                                                                                                                                                                       
+    def __init__(self, channel, queued=_QUEUED_SENTINEL):                                                                                                                                                      
         """                                                                                                                                                                                                    
         @param channel: the channel we're connected to.                                                                                                                                                        
-        @param queued: are we in the request queue, or can we start writing to                                                                                                                                 
-            the transport?                                                                                                                                                                                     
+        @param queued: (deprecated) are we in the request queue, or can we                                                                                                                                     
+            start writing to the transport?                                                                                                                                                                    
         """                                                                                                                                                                                                    
         self.notifications = []                                                                                                                                                                                
         self.channel = channel                                                                                                                                                                                 
-        self.queued = queued                                                                                                                                                                                   
+                                                                                                                                                                                                               
+        # Cache the client and server information, we'll need this                                                                                                                                             
+        # later to be serialized and sent with the request so CGIs                                                                                                                                             
+        # will work remotely                                                                                                                                                                                   
+        self.client = self.channel.getPeer()                                                                                                                                                                   
+        self.host = self.channel.getHost()                                                                                                                                                                     
+                                                                                                                                                                                                               
         self.requestHeaders = Headers()                                                                                                                                                                        
         self.received_cookies = {}                                                                                                                                                                             
         self.responseHeaders = Headers()                                                                                                                                                                       
         self.cookies = [] # outgoing cookies                                                                                                                                                                   
+        self.transport = self.channel.transport                                                                                                                                                                
                                                                                                                                                                                                                
-        if queued:                                                                                                                                                                                             
-            self.transport = StringTransport()                                                                                                                                                                 
-        else:                                                                                                                                                                                                  
-            self.transport = self.channel.transport                                                                                                                                                            
-                                                                                                                                                                                                               
-   
@@ -1018,31 +1130,113 @@
         self.sentLength = self.sentLength + len(data)
         if data:
             if self.chunked:
-                self.transport.writeSequence(toChunk(data))
+                self.channel.writeSequence(toChunk(data))
             else:
-                self.transport.write(data)
+                self.channel.write(data)

If this really is what is happening then I agree with Corentin that a few checks to see if self.channel is None will get rid of these tracebacks and defer the Runtime error to when .close is called. I'll put together a PR unless someone else gets to this first or tells me that something more subtle is going on here.

Last edited 3 years ago by jswitzer (previous) (diff)

comment:10 Changed 3 years ago by Glyph

Hi jswitzer!

Your analysis sounds vaguely correct; do you think you could write a unit test that validates this behavior? (This is particularly interesting because we need to make sure it does the right thing on both HTTP/1.1 and HTTP/2.0.)

comment:11 Changed 3 years ago by Daniel Pryor

comment:13 Changed 3 years ago by jswitzer

Can I ask for some help on my PR? AFAIK it's ready to go at the moment. Do I need to flag it somehow for a review?

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

Can I ask for some help on my PR? AFAIK it's ready to go at the moment. Do I need to flag it somehow for a review?

Yep. Put the "review" keyword on the ticket. See ReviewProcess.

comment:15 Changed 3 years ago by jswitzer

Keywords: review added

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

Owner: set to Jean-Paul Calderone

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

Keywords: review removed

Hello. Thanks for your contribution. Sorry about the delay in looking at it. I've left a review on GitHub. Unfortunately, I'm not really sure what we should do in this case so I've left a comment-only review (neither approving nor rejecting the PR). The gist is that I'm not clear on whether request.write raising an exception is a bug we can fix or an intended behavior we need to preserve (or go through the incompatibility process to change).

I'll try and start a discussion about that question and then revisit this.

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

FWIW, it looks like a small and WSGI-focused fix would be along these lines:

diff --git a/src/twisted/web/wsgi.py b/src/twisted/web/wsgi.py
index 311050f23..65677934f 100644
--- a/src/twisted/web/wsgi.py
+++ b/src/twisted/web/wsgi.py
@@ -504,10 +504,10 @@ class _WSGIResponse:
         try:
             appIterator = self.application(self.environ, self.startResponse)
             for elem in appIterator:
-                if elem:
-                    self.write(elem)
                 if self._requestFinished:
                     break
+                if elem:
+                    self.write(elem)
             close = getattr(appIterator, 'close', None)
             if close is not None:
                 close()

The test in the linked PR doesn't tell us if this works or not though since it is at the level of IRequest behavior and this fix is in the WSGI implementation.

comment:19 Changed 3 years ago by jswitzer

Keywords: review added

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

Keywords: review removed

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

Owner: changed from Jean-Paul Calderone to jswitzer

comment:22 Changed 3 years ago by Jean-Paul Calderone <exarkun@…>

Resolution: fixed
Status: newclosed

In c8d59d66:

Error: Processor CommitTicketReference failed
 does not appear to be a Git repository. See the log for more information.
Note: See TracTickets for help on using tickets.