Opened 7 years ago

Closed 5 years ago

#2677 defect closed fixed (fixed)

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

Reported by: exarkun Owned by:
Priority: high Milestone:
Component: web Keywords: tape
Cc: jack Branch: branches/finish-proxied-requests-2677
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

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 5 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 5 years ago by jack

  • Cc jack added

comment:3 Changed 5 years ago by glyph

  • Priority changed from normal to high

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

comment:4 Changed 5 years ago by glyph

  • Owner changed from jknight to exarkun

comment:5 Changed 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/finish-proxied-requests-2677

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

comment:6 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Done, enjoyably. Build results

comment:7 Changed 5 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 5 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

comment:9 Changed 5 years ago by exarkun

(In [26723]) Python 2.3 compat

refs #2677

comment:10 Changed 5 years ago by exarkun

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

refs #2677

comment:11 Changed 5 years ago by exarkun

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

refs #2677

comment:12 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun 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 5 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

Great, please merge.

comment:14 Changed 5 years ago by exarkun

(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 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun 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 5 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

Fair enough, +1.

comment:17 Changed 5 years ago by exarkun

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

(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 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.