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

[djfroofy]

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.

[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

[djfroofy]

Also, a related ticket for txaws in launchpad:

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

[itamar]

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

[exarkun]

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

[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.

[jonathanj]

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

[exarkun]

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

[jonathanj]

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

[jonathanj]

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

[therve]

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!

[itamar]

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

[itamar]

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

[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.

[sdsern]

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.

[radix]

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.