-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
transfer response handling #12480
transfer response handling #12480
Conversation
@monnerat, could I interest you in taking a glance at this PR? It is basically an extensions of the content encoder stack that, I believe, was done by you. Would be nice to get some feedback. |
@icing this merge conflicts now |
- use Curl_xfer_recv_resp() to receive response data from connection - used Curl_xfer_write_resp() to write response data to client
* code spelling in content_encoding.c * single-use functions made static
lib/content_encoding.c
Outdated
/* supported content encodings table. */ | ||
static const struct Curl_cwtype * const encodings[] = { | ||
/* supported general content decoders. */ | ||
static const struct Curl_cwtype * const general_decoders[] = { |
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.
When i wrote this, I preferred using "unencode" rather than "decode" to emphasize the context of encoding handling. So I'm not a big fan of the renaming.
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.
I can change that back. I thought "decode" was the opposite of "encode", but English is not my first language.
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.
It is. "unencode" was always very weird to read in my eyes. What does it mean? In English you encode one way and you decode the other way.
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.
I thought "decode" was the opposite of "encode"
Yes it is indeed. And the word "unencode" is not very elegant I admit.
This was just because we are dealing with encodings.
Thus I may be ok with decoding if you change it everywhare for consistency!
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.
Renamed as proposed.
lib/content_encoding.c
Outdated
@@ -851,6 +851,13 @@ static const struct Curl_cwtype * const encodings[] = { | |||
NULL | |||
}; | |||
|
|||
/* supported content decoders only for transfer encodings */ | |||
static const struct Curl_cwtype * const transfer_decoders[] = { |
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.
Wouldn't it be easier to keep a single table and add bit flags in the Curl_cwtype
structure to specify the allowed phase(s) and if it may be skipped or not? Just a suggestion.
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.
I prefer to keep them separate and void testing a flag while iterating. Matter of taste, probably.
The last PR in the "client writer" series. This clarifies the handling of server responses by folding the code for the complicated protocols into their protocol handlers. This concerns mainly HTTP and its bastard sibling RTSP.
Nomenclature
The terms "read" and "write" are often used without clear context if they refer to the connect or the client/application side of a transfer. This PR uses "read/write" for operations on the client side and "send/receive" for the connection, e.g. server side. If this is considered useful, we can revisit renaming of further methods in another PR.
Protocol Handler Interface
Curl's protocol handler
readwrite()
method been changed:The name was changed to clarify that this writes reponse data to the client side. The parameter changes are:
conn
removed as it always operates ondata->conn
pconsumed
removed as the method needs to handle all data on successreadmore
removed as no longer necessaryis_eos
as indicator that this is the last call for the transfer response (end-of-stream).done
TRUE on return iff the transfer response is to be treated as finishedThis change affects many files only because of updated comments in handlers that provide no implementation. The real change is that the HTTP protocol handlers now provide an implementation.
HTTP/RTSP write_resp()
The HTTP protocol handlers
write_resp()
implementation will get passed all raw data of a server response for the transfer. The HTTP/1.x formatted status and headers, as well as the undecoded response body.Curl_http_write_resp_hds()
is used internally to parse the response headers and pass them on. This method is public as the RTSP protocol handler also uses it.HTTP/1.1 "chunked" transport encoding is now part of the general content encoding writer stack, just like other encodings. A new flag
CLIENTWRITE_EOS
was added for the last client write. This allows writers to verify that they are in a valid end state. The chunked decoder will check if it indeed has seen the last chunk.General
transfer.c
handlingThe general response handling in
transfer.c:466
happens in functionreadwrite_data()
. This mainly operates now like:All the response data handling is implemented in
Curl_xfer_write_resp()
. It calls the protocol handler'swrite_resp()
implementation if available, or does the default behaviour.All raw response data needs to pass through this function. Which also means that anyone in possession of such data may call
Curl_xfer_write_resp()
. This was implemented for HTTP/2 in #12468 to demonstrate the effect this has on transfer handling.