Opened 11 years ago

Closed 6 years ago

#3697 release blocker: regression closed fixed (fixed)

Headers no longer available in requests passed over twisted.web.distrib

Reported by: Jean-Paul Calderone Owned by:
Priority: highest Milestone:
Component: web Keywords:
Cc: Jonathan Stoppani Branch: branches/distrib-request-headers-3697
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

twisted.web.distrib passes a Request object over PB to another process which handles the request and returns a response.

In 8.2, Request changed to include an attribute which cannot be serialized by PB (or it can be serialized, but not unserialized, perhaps). This results in an Unpersistable instance on the remote end of a distrib setup (confusingly, Unpersistable is what PB calls a problem where something cannot be unpersisted).

The attributes which cause problem are requestHeaders and responseHeaders. These are instances of twisted.web.http_headers.Headers which apparently don't interact with PB well.

Since this problem doesn't manifest as an exception during serialization or unserialization, the remote end will actually get a Request object and try to process it; however, this fails as soon as it tries to access any headers, since they are no longer available.

The failure on the remote end looks often like something like this:

2009-03-22 18:59:02-0400 [Broker,0,] <GET /builds HTTP/1.1>
2009-03-22 18:59:02-0400 [Broker,0,] Peer will receive following PB traceback:
2009-03-22 18:59:02-0400 [Broker,0,] Unhandled Error
        Traceback (most recent call last):
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/spread/banana.py", line 148, in gotItem
            self.callExpressionReceived(item)
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/spread/banana.py", line 113, in callExpressionReceived
            self.expressionReceived(obj)
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/spread/pb.py", line 522, in expressionReceived
            method(*sexp[1:])
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/spread/pb.py", line 833, in proto_message
            self._recvMessage(self.localObjectForID, requestID, objectID, message, answerRequired, netArgs, netKw)
        --- <exception caught here> ---
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/spread/pb.py", line 847, in _recvMessage
            netResult = object.remoteMessageReceived(self, message, netArgs, netKw)
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/spread/flavors.py", line 119, in remoteMessageReceived
            state = method(*args, **kw)
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/web/distrib.py", line 201, in remote_request
            return res.render(request)
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/web/static.py", line 304, in render
            return self.redirect(request)
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/web/static.py", line 366, in redirect
            return redirectTo(addSlash(request), request)
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/web/static.py", line 66, in addSlash
            return "http%s://%s%s/%s" % (
          File "/srv/bb-master/Projects/Twisted/trunk/twisted/web/http.py", line 674, in getHeader
            return self.received_headers.get(key.lower())
        exceptions.AttributeError: Unpersistable instance has no attribute 'get'

Change History (12)

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

Author: exarkun
Branch: branches/distrib-request-headers-3697

(In [26660]) Branching to 'distrib-request-headers-3697'

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

Keywords: review added
Owner: Jean-Paul Calderone deleted
Priority: highhighest

The branch fixes received_headers (by fixing requestHeaders). I didn't do anything with headers (the backwards compatibility name for responseHeaders), though. Setting things in headers directly in a distrib resource didn't previously work - the values would be silently ignored. You have to use setHeader, which wasn't broken by this change, since it bypasses the local headers dict entirely and just sends a message to set the header in the master process. This is kind of dumb, but at least it's not broken. In the future I think distrib should work differently (or be replaced by something which works differently) and doesn't have so much PB gunk mashed into it.

comment:3 Changed 11 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

One pyflake: twisted/web/server.py:17: 'time' imported but unused

Please merge.

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

(In [26695]) Remove unused import

refs #3697

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

Resolution: fixed
Status: newclosed

(In [26700]) Merge distrib-request-headers-3697

Author: exarkun Reviewer: therve Fixes: #3697

Handle the new Headers instance at twisted.web.http.Request.requestHeaders specially when serializing a Request over a PB connection (for example, when sending it to a distrib client) so that the values survive the transmission rather than morphing into an Unpersistable instance.

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

Resolution: fixed
Status: closedreopened

(In [26701]) Revert r26700 - new test fails on Python 2.3

Reopens #3697

On Python 2.3, dict.update does not accept a list of two-tuples, so the new test fails when it passes such a list to dict.update:

[ERROR]: twisted.web.test.test_distrib.DistribTest.test_requestHeaders

Traceback (most recent call last):
  File "/srv/bb-slave/Run/slave/full2.3/Twisted/twisted/spread/pb.py", line 840, in _recvMessage
    netResult = object.remoteMessageReceived(self, message, netArgs, netKw)
  File "/srv/bb-slave/Run/slave/full2.3/Twisted/twisted/spread/flavors.py", line 114, in remoteMessageReceived
    state = method(*args, **kw)
  File "/srv/bb-slave/Run/slave/full2.3/Twisted/twisted/web/distrib.py", line 198, in remote_request
    return res.render(request)
  File "/srv/bb-slave/Run/slave/full2.3/Twisted/twisted/web/test/test_distrib.py", line 93, in render
    requestHeaders.update(
exceptions.AttributeError: keys

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

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: reopenednew

Build results for the branch after a fix for the Python 2.3 issue in r26702 - http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/distrib-request-headers-3697

comment:8 Changed 11 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

The test failures indicate that twisted.web.test.test_webclient.WebClientSSLTestCase.test_downloadTimeout is intermittent. Want to file a ticket for that?

As far as this goes, though, thanks for the handy link to the builds. It looks fine to me: land it.

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

Resolution: fixed
Status: newclosed

(In [26710]) Merge distrib-request-headers-3697

Author: exarkun Reviewer: therve, glyph Fixes: #3697

Handle the new Headers instance at twisted.web.http.Request.requestHeaders specially when serializing a Request over a PB connection (for example, when sending it to a distrib client) so that the values survive the transmission rather than morphing into an Unpersistable instance. This re-merge of the branch fixes Python 2.3 compatibility.

comment:10 Changed 9 years ago by <automation>

Owner: Jean-Paul Calderone deleted

comment:11 Changed 6 years ago by Jonathan Stoppani

Cc: Jonathan Stoppani added
Resolution: fixed
Status: closedreopened

The description of the ticket mentions both requestHeaders and responseHeaders being problematic, but only requestHeaders are addressed in the branch. The same should be done for responseHeaders.

P.S.: Priority was "highest" on this one, I will leave it as is, but given that nobody encountered the problem in 4 years, we may want to set it to a lower value.

comment:12 Changed 6 years ago by Jean-Paul Calderone

Resolution: fixed
Status: reopenedclosed

Please file a new ticket; don't re-open old, fixed tickets.

Note: See TracTickets for help on using tickets.