From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 11 Mar 2019 13:02:21 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v2 1/1] http: fix httpc auto-managed headers Message-ID: <20190311100221.3oudvbsfaq4gxawb@esperanza> References: <9099f8a339c278e3a5dac683923d13c7ee470ce9.1550761676.git.kshcherbatov@tarantool.org> <80130a1b-84ad-5f90-9422-95bb0a1e67cb@tarantool.org> <998b800b-acc1-f0c8-1fd3-322d0c04f657@tarantool.org> <20190307135135.dner6r66szejw6lx@esperanza> <22400748-d82b-16ab-5809-68440cbfb1ef@tarantool.org> <20190307145328.ublvmnnyxvowgxl3@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Mon, Mar 11, 2019 at 12:23:02PM +0300, Kirill Shcherbatov wrote: > The https library intelligently sets the headers "Accept", > "Content-Length", "Connection", "Keep-Alive: timeout". Please remove "Content-Length" from the commit message. > However, when the user explicitly specified them in the header > options section of the call argument, they could be written to > the HTTP request twice. > We postponed the auto headers setup before https_exececute call. > Now they are set only if they were not set by the user. > > Closes #3955 > > https://github.com/tarantool/tarantool/tree/kshch/gh-3955-httpc-auto-managed-headers-redefine > https://github.com/tarantool/tarantool/issues/3955 > --- > src/httpc.c | 36 +++++++++++++++++++------------ > src/httpc.h | 21 ++++++++++++++++++ > test/app-tap/http_client.test.lua | 16 +++++++++++++- > 3 files changed, 58 insertions(+), 15 deletions(-) > > diff --git a/src/httpc.c b/src/httpc.c > index 04775b378..0615a32ec 100644 > --- a/src/httpc.c > +++ b/src/httpc.c > @@ -108,6 +108,8 @@ httpc_request_new(struct httpc_env *env, const char *method, > } > memset(req, 0, sizeof(*req)); > req->env = env; > + req->set_connection = true; > + req->set_kep_alive = true; > region_create(&req->resp_headers, &cord()->slabc); > region_create(&req->resp_body, &cord()->slabc); > > @@ -131,8 +133,7 @@ httpc_request_new(struct httpc_env *env, const char *method, > curl_easy_setopt(req->curl_request.easy, CURLOPT_POSTFIELDS, ""); > curl_easy_setopt(req->curl_request.easy, CURLOPT_POSTFIELDSIZE, 0); > curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method); > - if (httpc_set_header(req, "Accept: */*") < 0) > - goto error; > + req->set_accept_any_mime = true; > } else { > curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method); > } > @@ -152,9 +153,6 @@ httpc_request_new(struct httpc_env *env, const char *method, > ibuf_create(&req->body, &cord()->slabc, 1); > > return req; > -error: > - mempool_free(&env->req_pool, req); > - return NULL; > } > > void > @@ -185,6 +183,17 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...) > } > va_end(ap); > > + /** > + * Update flags for automanaged headers: no need to > + * set them automatically later. > + */ > + if (strncasecmp(header, "Accept:", 7) == 0) > + req->set_accept_any_mime = false; > + else if (strncasecmp(header, "Connection:", 11) == 0) > + req->set_connection = false; > + else if (strncasecmp(header, "Keep-Alive:", 11) == 0) > + req->set_kep_alive = false; > + Using a constant for string length looks error-prone. Please introduce constants for header literals and use them instead: strncasecmp(header, ACCEPT_HEADER, strlen(ACCEPT_HEADER)) > struct curl_slist *l = curl_slist_append(req->headers, header); > if (l == NULL) { > diag_set(OutOfMemory, strlen(header), "curl", "http header"); > @@ -223,15 +232,7 @@ httpc_set_keepalive(struct httpc_request *req, long idle, long interval) > curl_easy_setopt(req->curl_request.easy, CURLOPT_TCP_KEEPALIVE, 1L); > curl_easy_setopt(req->curl_request.easy, CURLOPT_TCP_KEEPIDLE, idle); > curl_easy_setopt(req->curl_request.easy, CURLOPT_TCP_KEEPINTVL, interval); > - if (httpc_set_header(req, "Connection: Keep-Alive") < 0 || > - httpc_set_header(req, "Keep-Alive: timeout=%d", > - (int) idle) < 0) { > - return -1; > - } > - } else { > - if (httpc_set_header(req, "Connection: close") < 0) { > - return -1; > - } > + req->idle = idle; > } httpc_set_keepalive never fails after this patch. Please remove the return value from the function signature. > #else > (void) req; > @@ -322,6 +323,13 @@ int > httpc_execute(struct httpc_request *req, double timeout) > { > struct httpc_env *env = req->env; > + if (req->set_connection && > + httpc_set_header(req, "Connection: %s", > + req->idle > 0 ? "Keep-Alive" : "close") != 0) > + return -1; > + if (req->set_kep_alive && req->idle > 0 && > + httpc_set_header(req, "Keep-Alive: timeout=%d", (int)req->idle) < 0) Please use %ld rather than (int) type conversion. > + return -1; Where do you set "Accept" header? > > curl_easy_setopt(req->curl_request.easy, CURLOPT_WRITEDATA, (void *) req); > curl_easy_setopt(req->curl_request.easy, CURLOPT_HEADERDATA, (void *) req); > diff --git a/src/httpc.h b/src/httpc.h > index b675d5f26..5c6bcc8a5 100644 > --- a/src/httpc.h > +++ b/src/httpc.h > @@ -109,6 +109,27 @@ struct httpc_request { > struct region resp_headers; > /** buffer of body */ > struct region resp_body; > + /** > + * Idle delay, in seconds, that the operating system will > + * wait while the connection is idle before sending > + * keepalive probes. > + */ > + long idle; keep_alive_timeout > + /** > + * True when connection header must be set before > + * execution automatically. > + */ > + bool set_connection; set_connection_header > + /** > + * True when accept Any MIME header must be set > + * before execution automatically. > + */ > + bool set_accept_any_mime; set_accept_header > + /** > + * True when Keep-Alive header must be set before > + * execution automatically. > + */ > + bool set_kep_alive; s/set_kep_alive/set_keep_alive_header > }; > > /** > diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua > index 3cecb47ed..563f1f67e 100755 > --- a/test/app-tap/http_client.test.lua > +++ b/test/app-tap/http_client.test.lua > @@ -80,6 +80,18 @@ local function test_http_client(test, url, opts) > test:is(r.status, 200, 'request') > end > > +-- > +-- gh-3955: Fix httpc auto-managed headers. > +-- Bad comment as it says nothing about what the test actually checks. Should be Check that httpc module doesn't redefine http headers set explicitly by the caller. > +local function test_http_client_headers_redefine(test, url, opts) > + test:plan(3) > + opts.headers={['Connection'] = 'Keep-Alive', ['Accept'] = 'text/html'} > + local r = client.get(url, opts) > + test:is(r.status, 200, 'simple 200') > + test:is(r.headers["accept"], 'text/html', 'redefined Accept header') > + test:is(r.headers["connection"], 'Keep-Alive', 'redefined Connection header') > +end > + Please add more test cases. In particular, you must check that if keepalive option contradicts http headers provided by the caller, httpc uses user-provided headers; if the user doesn't provide any headers, the defaults are used. > local function test_cancel_and_errinj(test, url, opts) > test:plan(3) > local ch = fiber.channel(1) > @@ -493,9 +505,11 @@ local function test_concurrent(test, url, opts) > end > > function run_tests(test, sock_family, sock_addr) > - test:plan(10) > + test:plan(11) > local server, url, opts = start_server(test, sock_family, sock_addr) > test:test("http.client", test_http_client, url, opts) > + test:test("http.client headers redefine", test_http_client_headers_redefine, > + url, opts) > test:test("cancel and errinj", test_cancel_and_errinj, url .. 'long_query', opts) > test:test("basic http post/get", test_post_and_get, url, opts) > test:test("errors", test_errors)