Opened 3 years ago

Closed 13 months ago

#5476 defect closed fixed (fixed)

Response.deliverBody() hangs on responses that have no body

Reported by: djfroofy Owned by: exarkun
Priority: normal Milestone:
Component: web Keywords: agent
Cc: jknight, jonathanj Branch: branches/deliverBody-no-body-5476-2
(diff, github, buildbot, log)
Author: jonathanj, exarkun Launchpad Bug:

Description

This might be a general case of not handling empty response bodies (vs. 0-length response bodies). This ticket is more of a placeholder for now. I'll review code and try to write isolated test case to emulate the behavior.

Note: I've seen this working on txaws tickets improved ec2 support (using new Agent class). What I've observed is the request seems to just hang after call to deliverBody() in the Response object; neither dataReceived() or connectionLost() are called on the receiver Protocol instance.

Attachments (3)

5476.patch (2.3 KB) - added by sdsern 19 months ago.
svn-sdsern-modified.patch (3.3 KB) - added by cscutcher 13 months ago.
Modified sdsern patch (Generated with git for svn)
svn-sdsern-modified-2.patch (3.3 KB) - added by cscutcher 13 months ago.
Modified sdsern patch (Generated with git for svn) [fixed]

Download all attachments as: .zip

Change History (30)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jknight added

comment:2 Changed 3 years ago by djfroofy

Relevant IRC conversation on this:

10:30  djfroofy: exarkun: do you have a second to chat about response.deliverBody() with methods like HEAD and DELETE that i mentioned yesterday (or was is 
                 yester-yesterday?)
10:30  djfroofy: i just wanted to understand things better before opening a ticket
10:31  exarkun: djfroofy: I have to run, actually.  Probably itamar remembers some things about that code, though.
10:31  ! AlekSi [~AlekSi@85.235.191.82] has quit [Remote host closed the connection]
10:31  djfroofy: exarkun: thanks, i'll ping itamar then
10:31  ! kevan_ [~kevan@host-134-71-248-23.allocated.csupomona.edu] has joined #twisted
10:32  djfroofy: itamar: see above. i'm thinking that for http methods where having a response body is violation of the spec, response.deliverBody() should, if 
                 possible, raise some exception
10:32  djfroofy: so i'm not an HTTP expert, but some methods at the top of my head are: HEAD and DELETE. although, it isn't clear to me whether delete allows a response 
                 body or not
10:37  idnar: djfroofy: I would imagine you can get an error with a response body from a DELETE
10:37  idnar: but I don't recall if rfc2616 says anything
10:38  djfroofy: idnar: so if I follow, you mean we should perhaps raise an exception call to deliverResponse() if method is DELETE? or HEAD for that matter?
10:38  idnar: uh, no, I meant an HTTP error
10:39  djfroofy: idnar: from my own experiments in the context of txaws, response.deliverBody() results in basically hanging for HEAD or DELETE.
10:39  djfroofy: i need to isolate this more and make a test case, of course
10:39  idnar: well, I would expect deliverBody to fail if there isn't a response body
10:39  idnar: I'm just saying that I think getting a response body from a DELETE request is possible, even if it shouldn't be for HEAD
10:39  lifeless: idnar: section 9.7
10:39  djfroofy: idnar: me too. but it just hangs indefinitely so far as I can tell. I think by the spec now it's possible for there to be a response body for DELETE
10:40  * idnar looks
10:40  lifeless: idnar: 200 for deleted-with-body, 204 for deleted-no-body and 202 for will-be-deleted-soon
10:40  idnar: djfroofy: presumably you're causing an exception somewhere by calling deliverBody when there isn't a body
10:40  itamar: possibly it should just deliver an empty body
10:40  idnar: the fact that there sometimes isn't a body doesn't mean that there will never be a body, though
10:41  djfroofy: idnar: right
10:41  idnar: lifeless: okay yeah, that's pretty clear
10:41  lifeless: and in fact if you're returning 200, it has to have a body
10:41  djfroofy: so i think checking the response codes is the right thing to do and not call deliverBody if we know there isn't one
10:42  djfroofy: i'm speaking from the perspective of a client calling deliverBody() to be clear
10:42  idnar: anyhow I don't want to say anything about the Agent API specifically because I haven't looked at the code recently
10:43  djfroofy: or ... the right thing to do is fix deliverBody to just deal with empty bodies without hanging; which i think is what's happening
10:44  djfroofy: idnar: ok, i'll just keep poking around then and try to come up with a test case. but at a higher level what do you imagine is the correct thing to do? 
                 handle empty bodies (no body) gracefully or raise exception if we know theres no body to be delivered?
10:44  idnar: that's what I meant about the API, I don't recall right now how everything fits together, so I'm not sure which way to go on that
10:45  idnar: lack of an entity isn't quite the same thing as a zero-length entity, so...
10:45  djfroofy: idnar: cool, i'll look into this in more detail then                 
11:34  itamar: djfroofy: further thought suggests that doing deliverBody with empty body should just deliver an empty body
11:35  itamar: otherwise any person using deliverBody has to think about the exception case explicitly, instead of just reusing the same code path
12:52  djfroofy: itamar: right, sorry i was away ... so i think maybe empty body should deliver None and 0-length body should deliver empty string. or do you think this 
                 difference matters?
12:52  djfroofy: or maybe a constant EMPTY_BODY

comment:3 Changed 3 years ago by djfroofy

Also, a related ticket for txaws in launchpad:

https://bugs.launchpad.net/txaws/+bug/924459

comment:4 Changed 3 years ago by therve

  • Summary changed from Response.devlierBody() from client appears to hang with HEAD and DELETE requests to Response.deliverBody() from client appears to hang with HEAD and DELETE requests

comment:5 Changed 3 years ago by itamar

I propose that dataReceived never be called, and connectionLost be called... perhaps with a NoBody which subclasses one of the existing exceptions.

comment:6 Changed 3 years ago by exarkun

Why the redundant signalling? Isn't dataReceived not being called sufficient?

comment:7 Changed 3 years ago by itamar

Mmmm... yeah, OK. Plus you get into these "is 0 length body the same as no body" philosophical discussions if you have an extra signal.

comment:8 Changed 2 years ago by jonathanj

  • Summary changed from Response.deliverBody() from client appears to hang with HEAD and DELETE requests to Response.deliverBody() hangs on responses that have no body

Since this ticket is actually about deliverBody hanging on responses that have no body, I'm altering the ticket's title.

comment:9 Changed 2 years ago by exarkun

Another possible API would be to raise an exception from deliverBody if there is no body.

comment:10 Changed 2 years ago by jonathanj

  • Author set to jonathanj
  • Branch set to branches/deliverBody-no-body-5476

(In [35876]) Branching to 'deliverBody-no-body-5476'

comment:11 Changed 2 years ago by jonathanj

(In [35877]) Call response._bodyDataFinished for responses with no body in the same way as responses with zero length bodies. refs #5476.

comment:12 Changed 2 years ago by jonathanj

  • Cc jonathanj added
  • Keywords agent review added
  • Owner djfroofy deleted

comment:13 Changed 2 years ago by therve

  • Keywords review removed
  • Owner set to jonathanj

It look good to me, but I feel that we should pass a custom reason to bodyDataFinished. The current message is "Response body fully received" which seems misleading in this case. Once that's done and tested please merge. Thanks!

comment:14 Changed 2 years ago by itamar

I would suggest it subclass "Response body fully received".

comment:15 Changed 2 years ago by itamar

Hm. Maybe ResponseDone with custom message is sufficient, so nevermind.

comment:16 Changed 19 months ago by jesstess

  • Owner changed from jonathanj to jesstess

comment:17 Changed 19 months ago by jesstess

  • Owner changed from jesstess to sdsern

comment:18 Changed 19 months ago by jonathanj

Something to think about:

The approach my branch takes is to make responses with no body behave the same way as responses with a zero-length body. There was some discussion on IRC about this behaviour potentially being misleading and how there are benefits to raising an exception upon calling deliverBody for empty-body responses, which in normal cases should not have deliverBody called on them, instead of silently merging zero-length and empty body behaviour.

I think I was convinced that raising an exception was the better approach.

Changed 19 months ago by sdsern

comment:19 Changed 19 months ago by sdsern

  • Keywords review added
  • Owner sdsern deleted

To differentiate between zero-length and empty body messages, the reason code for empty body messages is now "Response with no body fully received". Zero-length messages continue to use the "Response body fully received" reason code.

I modified the unit test to make sure the proper code for empty body messages was being passed to connectionLost.

comment:20 Changed 17 months ago by radix

  • Keywords review removed
  • Owner set to sdsern

I think this is fine (with the branch). We're not hiding information or making it ambiguous; if the user wants to know the details of the response they can examine the status. Raising an exception might be okay too, but I think that vs just calling connectionLost is a matter of bikeshedding.

[1] I don't think we should ever rely on English text to flag semantic differences in code. I think we should have a subclass to indicate the difference in semantics.

[2] sdsern: Why do you first define connectionLost as "pass" and then later replace the connectionLost implementation with one that appends to 'reason'?

I'm happy to have this branch and patch merged immediately once these two points are addressed; if there's a real reason for #2 then I'd like to read it first.

comment:21 Changed 16 months ago by shira

  • Owner changed from sdsern to shira
  • Status changed from new to assigned

Changed 13 months ago by cscutcher

Modified sdsern patch (Generated with git for svn)

Changed 13 months ago by cscutcher

Modified sdsern patch (Generated with git for svn) [fixed]

comment:22 Changed 13 months ago by cscutcher

  • Keywords review added
  • Owner shira deleted
  • Status changed from assigned to new

I'm currently blocked behind this issue and so am keen to get it resolved and merged.

I believe I have addressed radix's concerns in a modified version of sdsern's patch. I have added a subclass of ResponseDone to represent a zero-length response called NoBodyResponseDone and changed the unittest to check for this class instead of the message string. I have also restructured the test to remove the unnecessary "pass".

This is my first contribution and I'm not quite sure how you guys mix github and svn. To cover my bases I have made a pull request on the deliverBody-no-body-5476 branch on github and also attached a svn-style patch to this ticket with the same changes just in case.

As I say I'm blocked by this so I'd appreciate anything anyone can do to move this along quickly. If there's anything more I can do then please let me know.

Thanks in advance for any help.

comment:23 Changed 13 months ago by exarkun

I propose that dataReceived never be called, and connectionLost be called... perhaps with a NoBody which subclasses one of the existing exceptions.

Why the redundant signalling? Isn't dataReceived not being called sufficient?

Mmmm... yeah, OK. Plus you get into these "is 0 length body the same as no body" philosophical discussions if you have an extra signal.

It look good to me, but I feel that we should pass a custom reason to bodyDataFinished.

Well... I don't want to hurt your feelings - but can you please explain why introducing this redundant signaling path is a good idea?

As I suggested earlier, I think the redundancy is a needless complication that only makes the interface more confusing and introduces the possibility for new bugs (what should an application do when dataReceived is passed some bytes and then NoBodyResponseDone is passed to connectionLost?)

The current message is "Response body fully received" which seems misleading in this case.

The prose English message on the exception is useless, I agree. I'm sure it's only there as a (perhaps misguided) attempt to make logs a little more clear. Is this the only reason to create a subclass? How about deleting the message instead?

comment:24 Changed 13 months ago by cscutcher

@exarkun in retrospect I agree with you. You won me over with;

what should an application do when dataReceived is passed some bytes and then NoBodyResponseDone is passed to connectionLost

That would make the current github branch correct in your eyes? If nothing else it's easier to migrate from the simpler solution to the "different exception/message" solution if the mood changes, than the other way, and that would at least get things working. Can we get that branch merged?

comment:25 Changed 13 months ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

I prefer this formatting of the information from comment 2:

10:30 djfroofy: exarkun: do you have a second to chat about
response.deliverBody() with methods like HEAD and DELETE that i
mentioned yesterday (or was is yester-yesterday?)

10:30 djfroofy: i just wanted to understand things better before
opening a ticket

10:31 exarkun: djfroofy: I have to run, actually. Probably
itamar remembers some things about that code, though.

10:32 djfroofy: itamar: see above. i'm thinking that for http
methods where having a response body is violation of the spec,
response.deliverBody() should, if possible, raise some exception

10:32 djfroofy: so i'm not an HTTP expert, but some methods at
the top of my head are: HEAD and DELETE. although, it isn't clear
to me whether delete allows a response body or not

10:37 idnar: djfroofy: I would imagine you can get an error with
a response body from a DELETE

10:37 idnar: but I don't recall if rfc2616 says anything

10:38 djfroofy: idnar: so if I follow, you mean we should perhaps
raise an exception call to deliverResponse() if method is DELETE?
or HEAD for that matter?

10:38 idnar: uh, no, I meant an HTTP error

10:39 djfroofy: idnar: from my own experiments in the context of
txaws, response.deliverBody() results in basically hanging for
HEAD or DELETE.

10:39 djfroofy: i need to isolate this more and make a test case,
of course

10:39 idnar: well, I would expect deliverBody to fail if there
isn't a response body

10:39 idnar: I'm just saying that I think getting a response body
from a DELETE request is possible, even if it shouldn't be for
HEAD

10:39 lifeless: idnar: section 9.7

10:39 djfroofy: idnar: me too. but it just hangs indefinitely so
far as I can tell. I think by the spec now it's possible for
there to be a response body for DELETE

10:40 * idnar looks

10:40 lifeless: idnar: 200 for deleted-with-body, 204 for
deleted-no-body and 202 for will-be-deleted-soon

10:40 idnar: djfroofy: presumably you're causing an exception
somewhere by calling deliverBody when there isn't a body

10:40 itamar: possibly it should just deliver an empty body

10:40 idnar: the fact that there sometimes isn't a body doesn't
mean that there will never be a body, though

10:41 djfroofy: idnar: right

10:41 idnar: lifeless: okay yeah, that's pretty clear

10:41 lifeless: and in fact if you're returning 200, it has to
have a body

10:41 djfroofy: so i think checking the response codes is the
right thing to do and not call deliverBody if we know there isn't
one

10:42 djfroofy: i'm speaking from the perspective of a client
calling deliverBody() to be clear

10:42 idnar: anyhow I don't want to say anything about the Agent
API specifically because I haven't looked at the code recently

10:43 djfroofy: or ... the right thing to do is fix deliverBody
to just deal with empty bodies without hanging; which i think is
what's happening

10:44 djfroofy: idnar: ok, i'll just keep poking around then and
try to come up with a test case. but at a higher level what do
you imagine is the correct thing to do? handle empty bodies (no
body) gracefully or raise exception if we know theres no body to
be delivered?

10:44 idnar: that's what I meant about the API, I don't recall
right now how everything fits together, so I'm not sure which way
to go on that

10:45 idnar: lack of an entity isn't quite the same thing as a
zero-length entity, so...

10:45 djfroofy: idnar: cool, i'll look into this in more detail
then

11:34 itamar: djfroofy: further thought suggests that doing
deliverBody with empty body should just deliver an empty body

11:35 itamar: otherwise any person using deliverBody has to think
about the exception case explicitly, instead of just reusing the
same code path

12:52 djfroofy: itamar: right, sorry i was away ... so i think
maybe empty body should deliver None and 0-length body should
deliver empty string. or do you think this difference matters?

12:52 djfroofy: or maybe a constant EMPTY_BODY

Reviewing the branch, not any of the patches attached to this ticket now. Just a couple comments:

  1. twisted/web/_newclient.py - I'm suspicious of adding the line self.response._bodyDataFinished() after the line self._finished(self.clearLineBuffer()). The former has application-visible side-effects (probably calling a protocol's connectionLost method). The latter has HTTP11ClientProtocol-internal consequences (like parsing another response). This probably only matters if HTTP11ClientProtocol supports pipelining - which it doesn't (yet). Probably nothing actionable here, except perhaps to make a note in the implementation that this might lead to trouble later.
  2. twisted/web/test/test_newclient.py - There is a new use of assertEquals introduced - this should be avoided

These are straightforward changes and simple enough that I'm comfortable addressing them myself and then merging.

Thanks to everyone who contributed to this fix. It was great to have a lot of eyes on this tricky area of the code.

comment:26 Changed 13 months ago by exarkun

  • Author changed from jonathanj to jonathanj, exarkun
  • Branch changed from branches/deliverBody-no-body-5476 to branches/deliverBody-no-body-5476-2

(In [40187]) Branching to 'deliverBody-no-body-5476-2'

comment:27 Changed 13 months ago by exarkun

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

(In [40189]) Merge deliverBody-no-body-5476-2

Author: jonathanj, sdsern, cscutcher
Reviewer: therve, radix, exarkun
Fixes: #5476

When twisted.web.client.Response.deliverBody is used with a response with
no body, call the protocol's connectionLost method right away instead
of ignoring the protocol.

Note: See TracTickets for help on using tickets.