Opened 10 years ago

Closed 9 years ago

#2677 defect closed fixed (fixed)

twisted/web/proxy.py doesn't use IRequest.finish

Reported by: Jean-Paul Calderone Owned by:
Priority: high Milestone:
Component: web Keywords: tape
Cc: jack Branch: branches/finish-proxied-requests-2677
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

ProxyClient.handleResponseEnd uses loseConnection to disconnect the tcp session. It should be calling finish on the request, though.

Change History (18)

comment:1 Changed 9 years ago by jack

Keywords: tape added

Specifically this manifested for me as ReverseProxyResource requests not appearing in the logs. I have not found a work around.

As reverse proxies are becoming extremely common due to JavaScript's same origin policy, perhaps the priority of this bug should be stepped up a little?

comment:2 Changed 9 years ago by jack

Cc: jack added

comment:3 Changed 9 years ago by Glyph

Priority: normalhigh

Given how infrequently sponsors have actually requested things, I'm giving this a bump :).

comment:4 Changed 9 years ago by Glyph

Owner: changed from jknight to Jean-Paul Calderone

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

Author: exarkun
Branch: branches/finish-proxied-requests-2677

(In [26716]) Branching to 'finish-proxied-requests-2677'

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

Keywords: review added
Owner: Jean-Paul Calderone deleted

Done, enjoyably. Build results

comment:7 Changed 9 years ago by therve

  • There are some problems with 2.3, as you use sorted.
  • The _finished flag isn't tested, apparently.
  • Is the loseConnection call in handleResponseEnd still necessary? I suspect request.finish will do the right thing.

Thanks!

comment:8 Changed 9 years ago by therve

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

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

(In [26723]) Python 2.3 compat

refs #2677

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

(In [26724]) Add a test case which covers the duplicate protection in handleResponseEnd

refs #2677

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

(In [26725]) Add a test for the loseConnection call

refs #2677

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

Keywords: review added
Owner: Jean-Paul Calderone deleted

Thanks for the review!

  1. Replaced sorted with list.sort in r26723
  2. Added a test which covers _finished in r26724
  3. Added a test which requires the loseConnection call in order to pass in r26725 (it is necessary because it's the outgoing request's transport, whereas the request.finish() call will only deal with the incoming request's transport).

comment:13 Changed 9 years ago by therve

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

Great, please merge.

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

(In [26738]) Fix the error case test to assert something valid; fix the implementation to use the right request methods to report the error

refs #2677

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

Keywords: review added
Owner: Jean-Paul Calderone deleted

I made a mistake in one of the tests. It was asserting something nonsensical. Fixed in the above linked revision. Sorry about that.

comment:16 Changed 9 years ago by therve

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

Fair enough, +1.

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

Resolution: fixed
Status: newclosed

(In [26744]) Merge finish-proxied-requests-2677

Author: exarkun Reviewer: therve Fixes: #2677

Use the incoming IRequest object in the HTTP proxy when generating a response to it rather than using its undocumented transport object. This makes the proxy more generally useful (as it might support custom request objects now), fixes a bug which led to proxied requests not being logged, and simplifies the proxy code slightly be removing the need for it to know much about the protocol-level formatting of HTTP.

comment:18 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.