HTTP: implement trailing headers for chunked transfers #3350

aneutron · 2018-12-07T17:02:57Z

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

bagder

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

bagder · 2018-12-08T10:02:57Z

docs/libcurl/opts/CURLOPT_HTTP_TRAILER_DATA.3

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


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

bagder · 2018-12-08T10:04:10Z

docs/libcurl/opts/CURLOPT_HTTP_TRAILER_DATA.3

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


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

bagder · 2018-12-08T10:07:34Z

docs/libcurl/curl_easy_setopt.3

@@ -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


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.

bagder · 2018-12-08T10:08:17Z

docs/libcurl/opts/CURLOPT_HTTP_TRAILER_DATA.3

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


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

bagder · 2018-12-08T10:10:31Z

docs/libcurl/opts/CURLOPT_HTTP_TRAILER_FUNCTION.3

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


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

"set callback for sending trailing headers" perhaps?

bagder · 2018-12-08T10:22:08Z

lib/transfer.c

+  /* 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) {


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))

bagder · 2018-12-08T10:23:30Z

lib/transfer.c

+    else
+#endif
+      if((nread - hexlen) == 0 &&
+            data->state.trailers_state != HTTP_TRAILERS_INITIALIZED) {


this looks funnily indented

bagder · 2018-12-08T10:23:48Z

lib/urldata.h

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

+
+typedef enum {
+    HTTP_TRAILERS_NONE,


not two-space indentation

bagder · 2018-12-08T10:24:37Z

lib/urldata.h

@@ -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 */


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

bagder · 2018-12-08T10:25:35Z

lib/transfer.c

+{
+  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;


maybe add spaces to make this more readable?

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

aneutron · 2018-12-09T07:12:17Z

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.

mkauf · 2018-12-10T16:18:37Z

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

aneutron · 2018-12-10T16:23:04Z

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

bagder · 2018-12-12T14:22:19Z

Should I leave HTTP: in the commit message?

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

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 · 2018-12-14T09:12:17Z

Thanks!

aneutron · 2018-12-14T09:22:30Z

Thanks a lot ! 😄

aneutron force-pushed the http-trailers branch from 7ddc698 to 39379db Compare December 8, 2018 06:20

bagder added HTTP libcurl API labels Dec 8, 2018

bagder requested changes Dec 8, 2018

View reviewed changes

aneutron force-pushed the http-trailers branch 2 times, most recently from e1424c3 to b9bf487 Compare December 9, 2018 07:06

bagder approved these changes Dec 12, 2018

View reviewed changes

aneutron force-pushed the http-trailers branch from b9bf487 to 502dc09 Compare December 12, 2018 14:25

bagder closed this in f464535 Dec 14, 2018

aneutron deleted the http-trailers branch December 14, 2018 09:22

mkauf changed the title ~~HTTP: implement trailing headers for chuncked transfers~~ HTTP: implement trailing headers for chunked transfers Dec 14, 2018

lock bot locked as resolved and limited conversation to collaborators Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP: implement trailing headers for chunked transfers #3350

HTTP: implement trailing headers for chunked transfers #3350

aneutron commented Dec 7, 2018 •

edited

bagder left a comment

bagder Dec 8, 2018

bagder Dec 8, 2018

bagder Dec 8, 2018

bagder Dec 8, 2018

bagder Dec 8, 2018

bagder Dec 8, 2018

bagder Dec 8, 2018

bagder Dec 8, 2018

bagder Dec 8, 2018

bagder Dec 8, 2018

aneutron commented Dec 9, 2018 •

edited

mkauf commented Dec 10, 2018

aneutron commented Dec 10, 2018 •

edited

bagder commented Dec 12, 2018

bagder commented Dec 14, 2018

aneutron commented Dec 14, 2018

HTTP: implement trailing headers for chunked transfers #3350

HTTP: implement trailing headers for chunked transfers #3350

Conversation

aneutron commented Dec 7, 2018 • edited

bagder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aneutron commented Dec 9, 2018 • edited

mkauf commented Dec 10, 2018

aneutron commented Dec 10, 2018 • edited

bagder commented Dec 12, 2018

bagder commented Dec 14, 2018

aneutron commented Dec 14, 2018

aneutron commented Dec 7, 2018 •

edited

aneutron commented Dec 9, 2018 •

edited

aneutron commented Dec 10, 2018 •

edited