Fix issue #110 and issue #168. #217
Conversation
|
Unfortunately, I think this has to be done at the body parsing implementation. The message should not really know how to do this parsing when you ask for the body. I remember is code already in the HTTP client at master, before all the renaming and re-arranging happened. |
|
Ah, ok. I think it's in the correct place now (but I'm still wrapping my head around the structure of the codebase, so I might be wrong still). Thinking back on it, I'm not 100% sure that the PR to master has the code in the right place; I think I put it in the equivalent location in master as I did initially here (that is, the handling of chunked encoding was done when someone requests the body of the response as opposed to when we're setting the value of the promise). |
| @@ -373,6 +378,7 @@ | |||
| buffer_type part; | |||
| typename buffer_type::const_iterator part_begin; | |||
| string_type partial_parsed; | |||
| bool is_chunk_encoding; | |||
There was a problem hiding this comment.
This variable seems unnecessary. You only ever need to check this just before writing it into the message body once.
There was a problem hiding this comment.
Right, but without it I'm not sure how I'd get the header data to check if it's chunked encoding just before writing the message body. At that point, I have no reference to the response object, so I can't get the value stored in that future (and if I get a future from the promise that I do have access to, I cause issues because there can only be one future for any given promise).
I suppose I could try to look for that value as we're building up the buffer to parse the headers out with, but that felt more hacky than doing it this way, where I definitely have access to the headers that are about to be put into the promise.
Maybe I'm overlooking a better way, though?
|
Thanks -- I'm not too worried about the change in master since it's going to be re-written anyway to the new API/implementation. If you'd still like to take that on in master too I would be happy to review that change. |
|
LGTM |
Similar to the prior PR to master, but for the 0.9.x versions.
Fixes issue #110 and issue #168.