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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

The http library intelligently sets the headers "Accept",
"Connection", "Keep-Alive".
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                       | 52 +++++++++++++++++++++----------
 src/httpc.h                       | 23 +++++++++++++-
 src/lua/httpc.c                   |  6 +---
 test/app-tap/http_client.test.lua | 36 ++++++++++++++++++++-
 4 files changed, 94 insertions(+), 23 deletions(-)

diff --git a/src/httpc.c b/src/httpc.c
index 04775b378..4651eaa72 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -39,6 +39,11 @@
 
 #define MAX_HEADER_LEN 8192
 
+/** The HTTP headers that may be set automatically. */
+#define HTTP_ACCEPT_HEADER	"Accept:"
+#define HTTP_CONNECTION_HEADER	"Connection:"
+#define HTTP_KEEP_ALIVE_HEADER	"Keep-Alive:"
+
 /**
  * libcurl callback for CURLOPT_WRITEFUNCTION
  * @see https://curl.haxx.se/libcurl/c/CURLOPT_WRITEFUNCTION.html
@@ -108,6 +113,8 @@ httpc_request_new(struct httpc_env *env, const char *method,
 	}
 	memset(req, 0, sizeof(*req));
 	req->env = env;
+	req->set_connection_header = true;
+	req->set_keep_alive_header = true;
 	region_create(&req->resp_headers, &cord()->slabc);
 	region_create(&req->resp_body, &cord()->slabc);
 
@@ -131,8 +138,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_header = true;
 	} else {
 		curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
 	}
@@ -152,9 +158,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 +188,20 @@ 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, HTTP_ACCEPT_HEADER,
+			strlen(HTTP_ACCEPT_HEADER)) == 0)
+		req->set_accept_header = false;
+	else if (strncasecmp(header, HTTP_CONNECTION_HEADER,
+		 strlen(HTTP_CONNECTION_HEADER)) == 0)
+		req->set_connection_header = false;
+	else if (strncasecmp(header, HTTP_KEEP_ALIVE_HEADER,
+			     strlen(HTTP_KEEP_ALIVE_HEADER)) == 0)
+		req->set_keep_alive_header = false;
+
 	struct curl_slist *l = curl_slist_append(req->headers, header);
 	if (l == NULL) {
 		diag_set(OutOfMemory, strlen(header), "curl", "http header");
@@ -214,7 +231,7 @@ httpc_set_body(struct httpc_request *req, const char *body, size_t size)
 	return 0;
 }
 
-int
+void
 httpc_set_keepalive(struct httpc_request *req, long idle, long interval)
 {
 #if LIBCURL_VERSION_NUM >= 0x071900
@@ -223,22 +240,13 @@ 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->keep_alive_timeout = idle;
 	}
 #else
 	(void) req;
 	(void) idle;
 	(void) interval;
 #endif
-	return 0;
 }
 
 void
@@ -322,6 +330,18 @@ int
 httpc_execute(struct httpc_request *req, double timeout)
 {
 	struct httpc_env *env = req->env;
+	if (req->set_accept_header &&
+	    httpc_set_header(req, "Accept: */*") < 0)
+		return -1;
+	if (req->set_connection_header &&
+	    httpc_set_header(req, "Connection: %s",
+			     req->keep_alive_timeout > 0 ?
+			     "Keep-Alive" : "close") != 0)
+		return -1;
+	if (req->set_keep_alive_header && req->keep_alive_timeout > 0 &&
+	    httpc_set_header(req, "Keep-Alive: timeout=%ld",
+			     req->keep_alive_timeout) < 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..821c73955 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 keep_alive_timeout;
+	/**
+	 * True when connection header must be set before
+	 * execution automatically.
+	 */
+	bool set_connection_header;
+	/**
+	 * True when accept Any MIME header must be set
+	 * before execution automatically.
+	 */
+	bool set_accept_header;
+	/**
+	 * True when Keep-Alive header must be set before
+	 * execution automatically.
+	 */
+	bool set_keep_alive_header;
 };
 
 /**
@@ -156,7 +177,7 @@ httpc_set_body(struct httpc_request *req, const char *body, size_t size);
  * @details Does nothing on libcurl < 7.25.0
  * @see https://curl.haxx.se/libcurl/c/CURLOPT_TCP_KEEPALIVE.html
  */
-int
+void
 httpc_set_keepalive(struct httpc_request *req, long idle, long interval);
 
 /**
diff --git a/src/lua/httpc.c b/src/lua/httpc.c
index 3a48cb31a..706b9d90b 100644
--- a/src/lua/httpc.c
+++ b/src/lua/httpc.c
@@ -253,11 +253,7 @@ luaT_httpc_request(lua_State *L)
 		keepalive_interval = (long) lua_tonumber(L, -1);
 	lua_pop(L, 1);
 
-	if (httpc_set_keepalive(req, keepalive_idle,
-				keepalive_interval) < 0) {
-		httpc_request_delete(req);
-		return luaT_error(L);
-	}
+	httpc_set_keepalive(req, keepalive_idle, keepalive_interval);
 
 	lua_getfield(L, 5, "low_speed_limit");
 	if (!lua_isnil(L, -1))
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 3cecb47ed..0a323be9b 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -80,6 +80,38 @@ local function test_http_client(test, url, opts)
     test:is(r.status, 200, 'request')
 end
 
+--
+-- gh-3955: 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(9)
+    local opts = table.deepcopy(opts)
+    -- Test defaults
+    opts.headers = {['Connection'] = nil, ['Accept'] = nil}
+    local r = client.post(url, nil, opts)
+    test:is(r.status, 200, 'simple 200')
+    test:is(r.headers['connection'], 'close', 'Default Connection header')
+    test:is(r.headers['accept'], '*/*', 'Default Accept header for POST request')
+    -- Test that in case of conflicting headers, user variant is
+    -- prefered
+    opts.headers={['Connection'] = 'close'}
+    opts.keepalive_idle = 2
+    opts.keepalive_interval = 1
+    local r = client.get(url, opts)
+    test:is(r.status, 200, 'simple 200')
+    test:is(r.headers['connection'], 'close', 'Redefined Connection header')
+    test:is(r.headers['keep_alive'], 'timeout=2',
+            'Automatically set Keep-Alive header')
+    -- Test that user-defined Connection and Acept headers
+    -- are used
+    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 +525,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