- Notifications
You must be signed in to change notification settings - Fork425
Fix issue #110 and issue #168.#217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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. Ithink 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 Ido 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, yes, you're right.
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.