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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

The https library intelligently sets the headers "Accept",
"Content-Length", "Connection", "Keep-Alive: timeout".
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;
+
 	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;
 	}
 #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)
+		return -1;
 
 	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;
+	/**
+	 * True when connection header must be set before
+	 * execution automatically.
+	 */
+	bool set_connection;
+	/**
+	 * True when accept Any MIME header must be set
+	 * before execution automatically.
+	 */
+	bool set_accept_any_mime;
+	/**
+	 * True when Keep-Alive header must be set before
+	 * execution automatically.
+	 */
+	bool set_kep_alive;
 };
 
 /**
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.
+--
+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
+
 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)
-- 
2.21.0