[tarantool-patches] Re: [PATCH v2 1/1] http: fix httpc auto-managed headers
Vladimir Davydov
vdavydov.dev at gmail.com
Mon Mar 11 13:02:21 MSK 2019
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)
More information about the Tarantool-patches
mailing list