Re: [tarantool-patches] Re: [PATCH v2 1/1] http: fix httpc auto-managed headers

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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)