Skip to content
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

HTTP: implement trailing headers for chunked transfers #3350

Closed
wants to merge 1 commit into from

Conversation

@aneutron
Copy link
Contributor

@aneutron aneutron commented Dec 7, 2018

This patch adds the CURLOPT_TRAILERDATA and CURLOPT_TRAILERFUNCTION options that allow a callback based approach to sending trailing headers with chunked transfers.

Following the everything is a state machine mantra, it's four stages state machine that traps right before sending the final CRLF, calls the trailer function that fills a curl_slist with the trailers, compiles them into a single buffer, and proceeds to read from the buffer as if reading from the read callback, then jumps back into the next main state as if nothing happened.

The test server (sws) was updated to take into account the detection of the end of transfer in the case of trailing headers presence.

Test 1591 checks that trailing headers can be sent using libcurl.

(I'm very sorry I had to remake this PR, the first one was made to merge my master branch into the curl master branch, it was a rookie mistake that made rebasing difficult ... That and the lack of tests, half-baked docs and tests that didn't pass. Suffice to say that mistakes were made. I sincerely apologize. All the remarks on the previous PR were addressed.)

Copy link
Member

@bagder bagder left a comment

Nice work! I've mostly found minor nits and details to remark on.

Loading

.\" * | (__| |_| | _ <| |___
.\" * \___|\___/|_| \_\_____|
.\" *
.\" * Copyright (C) 1998 - 2017, Daniel Stenberg, <daniel@haxx.se>, et al.
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielgustafsson it seems the travis check for copyright year mismatch doesn't work on new files?

Loading

.TH CURLOPT_HTTP_TRAILER_DATA 3 "14 Aug 2018" "libcurl 7.??.?" "curl_easy_setopt options"

.SH NAME:
CURLOPT_HTTP_TRAILER_DATA \- Set user data to be passed on to the trailing headers callback
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"custom pointer passed to the trailing headers callback" maybe? We want it to be short and sweet. It also makes sense to have all *DATA options described using similar wording.

Loading

@@ -327,6 +327,12 @@ Disable Content decoding. See \fICURLOPT_HTTP_CONTENT_DECODING(3)\fP
Disable Transfer decoding. See \fICURLOPT_HTTP_TRANSFER_DECODING(3)\fP
.IP CURLOPT_EXPECT_100_TIMEOUT_MS
100-continue timeout. See \fICURLOPT_EXPECT_100_TIMEOUT_MS(3)\fP
.IP CURLOPT_HTTP_TRAILER_FUNCTION
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bikeshedding here perhaps, but...

I'm not convinced it is a good idea to include HTTP in the name. What if there's another protocol with a similar concept that we could use this callback for at a later point in time? Maybe just CURLOPT_TRAILERFUNCTION ? Most of the existing *FUNCTION names are already named without a separating underscore.

Loading


.SH EXAMPLE:
.nf
// Assuming we have a CURL handle in the hndl variable.
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we discourage // comments everywhere (for C89 portability), we probably should use them in the man page example either...

Loading

.TH CURLOPT_HTTP_TRAILER_FUNCTION 3 "14 Aug 2018" "libcurl 7.63.0" "curl_easy_setopt options"

.SH NAME:
CURLOPT_HTTP_TRAILER_FUNCTION \- Callback to fill trailing headers list to be sent in a chunked transfer.
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too I think this should be as short as possible that still conveys what it does:

"set callback for sending trailing headers" perhaps?

Loading

lib/transfer.c Outdated
/* if we are transmitting trailing data, we don't need to write
a chunk size so we skip this */
if(data->req.upload_chunky
&& data->state.trailers_state == HTTP_TRAILERS_NONE) {
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When splitting long expressions, we usually prefer to split the lines with the operator on the previous line, and I would recommend parentheses:

(data->req.upload_chunky &&
 (data->state.trailers_state == HTTP_TRAILERS_NONE))

Loading

lib/transfer.c Outdated
else
#endif
if((nread - hexlen) == 0 &&
data->state.trailers_state != HTTP_TRAILERS_INITIALIZED) {
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks funnily indented

Loading

lib/urldata.h Outdated
@@ -1216,6 +1216,15 @@ typedef enum {
EXPIRE_LAST /* not an actual timer, used as a marker only */
} expire_id;


typedef enum {
HTTP_TRAILERS_NONE,
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not two-space indentation

Loading

lib/urldata.h Outdated
@@ -1362,6 +1371,13 @@ struct UrlState {
#endif
CURLU *uh; /* URL handle for the current parsed URL */
struct urlpieces up;
#ifndef CURL_DISABLE_HTTP
size_t trailers_bytes_sent; /* no other way of keeping track of sent bytes */
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no point in explaining what it isn't...

Loading

lib/transfer.c Outdated
{
struct Curl_easy *data = (struct Curl_easy *)raw;
Curl_send_buffer *trailers_buf = data->state.trailers_buf;
return trailers_buf->size_used-data->state.trailers_bytes_sent;
Copy link
Member

@bagder bagder Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add spaces to make this more readable?

return trailers_buf->size_used - data->state.trailers_bytes_sent;

Loading

@aneutron aneutron force-pushed the http-trailers branch 2 times, most recently from e1424c3 to b9bf487 Dec 9, 2018
@aneutron
Copy link
Contributor Author

@aneutron aneutron commented Dec 9, 2018

I think I addressed all the remarks:

  • Replaced CULROPT_HTTP_TRAILER* with CURLOPT_TRAILER*
  • Renamed the trailer state enum members from HTTP_TRAILER_* to TRAILER_*
  • Added two defs (CURL_TRAILERFUNC_OK & CURL_TRAILERFUNC_ABORT) to be used as return codes for the trailer function that now returns an integer.
  • Fixed the dates and releases on the docs, and edited the docs as suggested.
  • Fixed the weird indentations that slipped.
  • (I'm sorry I just noticed this a day after I addressed everything else) Fixed the commit message to reflect these changes.

Loading

@mkauf
Copy link
Contributor

@mkauf mkauf commented Dec 10, 2018

Please fix the typo in the commit message: "chuncked" --> "chunked".

Loading

@aneutron
Copy link
Contributor Author

@aneutron aneutron commented Dec 10, 2018

@mkauf Thanks for spotting that !
@bagder Should I leave HTTP: in the commit message ?

Loading

@bagder
Copy link
Member

@bagder bagder commented Dec 12, 2018

Should I leave HTTP: in the commit message?

Yes I think so, since this is after all only for HTTP (now).

Loading

bagder
bagder approved these changes Dec 12, 2018
This adds the CURLOPT_TRAILERDATA and CURLOPT_TRAILERFUNCTION
options that allow a callback based approach to sending trailing headers
with chunked transfers.

The test server (sws) was updated to take into account the detection of the
end of transfer in the case of trailing headers presence.

Test 1591 checks that trailing headers can be sent using libcurl.
@bagder
Copy link
Member

@bagder bagder commented Dec 14, 2018

Thanks!

Loading

@aneutron
Copy link
Contributor Author

@aneutron aneutron commented Dec 14, 2018

Thanks a lot ! 😄

Loading

@aneutron aneutron deleted the http-trailers branch Dec 14, 2018
@mkauf mkauf changed the title HTTP: implement trailing headers for chuncked transfers HTTP: implement trailing headers for chunked transfers Dec 14, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants