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

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

[Kirill Shcherbatov]

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.
Was introduced service http c_managed_header_mask mask in which
the bits corresponding to the http auto-manager headers, set 1
when referenced. When the bit is set, the new value is not
applied. In this way, we prefer user-defined value for all
httpc-managed headers.

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                       | 41 +++++++++++++++++++++++++++++++
 src/httpc.h                       |  8 ++++++
 test/app-tap/http_client.test.lua | 12 ++++++++-
 3 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/src/httpc.c b/src/httpc.c
index 950f8b32f..a04225d32 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -170,6 +170,42 @@ httpc_request_delete(struct httpc_request *req)
 	mempool_free(&req->env->req_pool, req);
 }
 
+/**
+ * Update bitmask of the http request headers that httpc may set
+ * automatically. In case of reserved pattern is found in header,
+ * routine sets corresponding bit in auto_headers_mask.
+ * Returns -1 when header is reserved and it's bit is already set
+ * in auto_headers_mask; 0 otherwise,
+*/
+static int
+httpc_managed_headers_mask_update(uint8_t *auto_headers_mask,
+				  const char *header)
+{
+	static struct {
+		const char *header_name;
+		uint32_t header_name_len;
+		int8_t bit_index;
+	} httpc_managed_headers[] = {
+		{"Connection",		10,	1},
+		{"Keep-Alive: timeout", 19,	2},
+		{"Content-Length",	14,	4},
+		{"Accept",		6, 	8},
+	};
+	uint32_t httpc_managed_headers_cnt =
+		sizeof(httpc_managed_headers)/sizeof(httpc_managed_headers[0]);
+	for (uint32_t i = 0; i < httpc_managed_headers_cnt; i++) {
+		if (strncasecmp(header, httpc_managed_headers[i].header_name,
+				httpc_managed_headers[i].header_name_len) != 0)
+			continue;
+		if ((*auto_headers_mask &
+		     httpc_managed_headers[i].bit_index) != 0)
+			return -1;
+		*auto_headers_mask |= httpc_managed_headers[i].bit_index;
+		return 0;
+	}
+	return 0;
+}
+
 int
 httpc_set_header(struct httpc_request *req, const char *fmt, ...)
 {
@@ -178,6 +214,11 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...)
 	const char *header = tt_vsprintf(fmt, ap);
 	va_end(ap);
 
+	if (httpc_managed_headers_mask_update(&req->auto_headers_mask,
+					      header) != 0) {
+		/* Do not add extra header to list. */
+		return 0;
+	}
 	struct curl_slist *l = curl_slist_append(req->headers, header);
 	if (l == NULL) {
 		diag_set(OutOfMemory, strlen(header), "curl", "http header");
diff --git a/src/httpc.h b/src/httpc.h
index dc709e6fa..d30343be1 100644
--- a/src/httpc.h
+++ b/src/httpc.h
@@ -109,6 +109,14 @@ struct httpc_request {
 	struct region resp_headers;
 	/** buffer of body */
 	struct region resp_body;
+	/**
+	 * A bitmask of the http request headers from among
+	 * the headers that httpc can set automatically.
+	 * This allows to prevent settings such headers twice.
+	 * Read httpc_managed_headers_mask_update definition for
+	 * more details.
+	 */
+	uint8_t auto_headers_mask;
 };
 
 /**
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 10a3728f8..8d1c50549 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -79,6 +79,14 @@ local function test_http_client(test, url, opts)
     test:is(r.status, 200, 'request')
 end
 
+local function test_http_client_headers_redefine(test, url, opts)
+    test:plan(2)
+    opts.headers={['Connection'] = 'Keep-Alive'}
+    local r = client.get(url, opts)
+    test:is(r.status, 200, 'simple 200')
+    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)
@@ -397,9 +405,11 @@ local function test_concurrent(test, url, opts)
 end
 
 function run_tests(test, sock_family, sock_addr)
-    test:plan(9)
+    test:plan(10)
     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.20.1

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

[Kirill Shcherbatov]

Perhaps even better here is so:

diff --git a/src/httpc.c b/src/httpc.c
index b4ca6cfce..f2df069e5 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -184,23 +184,24 @@ httpc_managed_headers_mask_update(uint8_t *auto_headers_mask,
 	static struct {
 		const char *header_name;
 		uint32_t header_name_len;
-		int8_t bit_index;
 	} httpc_managed_headers[] = {
-		{"Connection",		10,	1},
-		{"Keep-Alive",		10,	2},
-		{"Content-Length",	14,	4},
-		{"Accept",		6, 	8},
+		{"Connection",		10},
+		{"Keep-Alive",		10},
+		{"Content-Length",	14},
+		{"Accept",		6},
 	};
 	uint32_t httpc_managed_headers_cnt =
 		sizeof(httpc_managed_headers)/sizeof(httpc_managed_headers[0]);
+	assert(httpc_managed_headers_cnt <
+	       sizeof(*auto_headers_mask) * CHAR_BIT);
 	for (uint32_t i = 0; i < httpc_managed_headers_cnt; i++) {
 		if (strncasecmp(header, httpc_managed_headers[i].header_name,
 				httpc_managed_headers[i].header_name_len) != 0)
 			continue;
-		if ((*auto_headers_mask &
-		     httpc_managed_headers[i].bit_index) != 0)
+		int8_t bit_index = 1 << i;
+		if ((*auto_headers_mask & bit_index) != 0)
 			return -1;
-		*auto_headers_mask |= httpc_managed_headers[i].bit_index;
+		*auto_headers_mask |= bit_index;
 		return 0;
 	}
 	return 0;


===========================================

ntent-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.
Was introduced service http c_managed_header_mask mask in which
the bits corresponding to the http auto-manager headers, set 1
when referenced. When the bit is set, the new value is not
applied. In this way, we prefer user-defined value for all
httpc-managed headers.

Closes #3955
---
 src/httpc.c                       | 42 +++++++++++++++++++++++++++++++
 src/httpc.h                       |  8 ++++++
 test/app-tap/http_client.test.lua | 12 ++++++++-
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/src/httpc.c b/src/httpc.c
index 950f8b32f..f2df069e5 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -170,6 +170,43 @@ httpc_request_delete(struct httpc_request *req)
 	mempool_free(&req->env->req_pool, req);
 }
 
+/**
+ * Update bitmask of the http request headers that httpc may set
+ * automatically. In case of reserved pattern is found in header,
+ * routine sets corresponding bit in auto_headers_mask.
+ * Returns -1 when header is reserved and it's bit is already set
+ * in auto_headers_mask; 0 otherwise,
+*/
+static int
+httpc_managed_headers_mask_update(uint8_t *auto_headers_mask,
+				  const char *header)
+{
+	static struct {
+		const char *header_name;
+		uint32_t header_name_len;
+	} httpc_managed_headers[] = {
+		{"Connection",		10},
+		{"Keep-Alive",		10},
+		{"Content-Length",	14},
+		{"Accept",		6},
+	};
+	uint32_t httpc_managed_headers_cnt =
+		sizeof(httpc_managed_headers)/sizeof(httpc_managed_headers[0]);
+	assert(httpc_managed_headers_cnt <
+	       sizeof(*auto_headers_mask) * CHAR_BIT);
+	for (uint32_t i = 0; i < httpc_managed_headers_cnt; i++) {
+		if (strncasecmp(header, httpc_managed_headers[i].header_name,
+				httpc_managed_headers[i].header_name_len) != 0)
+			continue;
+		int8_t bit_index = 1 << i;
+		if ((*auto_headers_mask & bit_index) != 0)
+			return -1;
+		*auto_headers_mask |= bit_index;
+		return 0;
+	}
+	return 0;
+}
+
 int
 httpc_set_header(struct httpc_request *req, const char *fmt, ...)
 {
@@ -178,6 +215,11 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...)
 	const char *header = tt_vsprintf(fmt, ap);
 	va_end(ap);
 
+	if (httpc_managed_headers_mask_update(&req->auto_headers_mask,
+					      header) != 0) {
+		/* Do not add extra header to list. */
+		return 0;
+	}
 	struct curl_slist *l = curl_slist_append(req->headers, header);
 	if (l == NULL) {
 		diag_set(OutOfMemory, strlen(header), "curl", "http header");
diff --git a/src/httpc.h b/src/httpc.h
index dc709e6fa..d30343be1 100644
--- a/src/httpc.h
+++ b/src/httpc.h
@@ -109,6 +109,14 @@ struct httpc_request {
 	struct region resp_headers;
 	/** buffer of body */
 	struct region resp_body;
+	/**
+	 * A bitmask of the http request headers from among
+	 * the headers that httpc can set automatically.
+	 * This allows to prevent settings such headers twice.
+	 * Read httpc_managed_headers_mask_update definition for
+	 * more details.
+	 */
+	uint8_t auto_headers_mask;
 };
 
 /**
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 10a3728f8..8d1c50549 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -79,6 +79,14 @@ local function test_http_client(test, url, opts)
     test:is(r.status, 200, 'request')
 end
 
+local function test_http_client_headers_redefine(test, url, opts)
+    test:plan(2)
+    opts.headers={['Connection'] = 'Keep-Alive'}
+    local r = client.get(url, opts)
+    test:is(r.status, 200, 'simple 200')
+    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)
@@ -397,9 +405,11 @@ local function test_concurrent(test, url, opts)
 end
 
 function run_tests(test, sock_family, sock_addr)
-    test:plan(9)
+    test:plan(10)
     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.20.1

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

[Kirill Shcherbatov]

I've reworked code a bit to set all user-defined headers before other httpc
methods call to they have a priority.
================================================

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.
Was introduced service http c_managed_header_mask mask in which
the bits corresponding to the http auto-manager headers, set 1
when referenced. When the bit is set, the new value is not
applied. In this way, we prefer user-defined value for all
httpc-managed headers.

Closes #3955
---
src/httpc.c | 123 +++++++++++++++++++++---------
src/httpc.h | 24 +++++-
src/lua/httpc.c | 45 ++++++-----
test/app-tap/http_client.test.lua | 13 +++-
4 files changed, 145 insertions(+), 60 deletions(-)

diff --git a/src/httpc.c b/src/httpc.c
index 950f8b32f..4685a712a 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -95,8 +95,7 @@ httpc_env_destroy(struct httpc_env *ctx)
}

struct httpc_request *
-httpc_request_new(struct httpc_env *env, const char *method,
- const char *url)
+httpc_request_new(struct httpc_env *env)
{
struct httpc_request *req = mempool_alloc(&env->req_pool);
if (req == NULL) {
@@ -110,42 +109,7 @@ httpc_request_new(struct httpc_env *env, const char *method,
region_create(&req->resp_body, &cord()->slabc);

if (curl_request_create(&req->curl_request) != 0)
- return NULL;
-
- if (strcmp(method, "GET") == 0) {
- curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L);
- } else if (strcmp(method, "HEAD") == 0) {
- curl_easy_setopt(req->curl_request.easy, CURLOPT_NOBODY, 1L);
- } else if (strcmp(method, "POST") == 0 ||
- strcmp(method, "PUT") == 0 ||
- strcmp(method, "PATCH")) {
- /*
- * Set CURLOPT_POSTFIELDS to "" and CURLOPT_POSTFIELDSSIZE 0
- * to avoid the read callback in any cases even if user
- * forgot to call httpc_set_body() for POST request.
- * @see https://curl.haxx.se/libcurl/c/CURLOPT_POSTFIELDS.html
- */
- curl_easy_setopt(req->curl_request.easy, CURLOPT_POST, 1L);
- 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;
- } else {
- curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
- }
-
- curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
-
- curl_easy_setopt(req->curl_request.easy, CURLOPT_FOLLOWLOCATION, 1);
- curl_easy_setopt(req->curl_request.easy, CURLOPT_SSL_VERIFYPEER, 1);
- curl_easy_setopt(req->curl_request.easy, CURLOPT_WRITEFUNCTION,
- curl_easy_write_cb);
- curl_easy_setopt(req->curl_request.easy, CURLOPT_HEADERFUNCTION,
- curl_easy_header_cb);
- curl_easy_setopt(req->curl_request.easy, CURLOPT_NOPROGRESS, 1L);
- curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTP_VERSION,
- CURL_HTTP_VERSION_1_1);
+ goto error;

ibuf_create(&req->body, &cord()->slabc, 1);

@@ -170,6 +134,43 @@ httpc_request_delete(struct httpc_request *req)
mempool_free(&req->env->req_pool, req);
}

+/**
+ * Update bitmask of the http request headers that httpc may set
+ * automatically. In case of reserved pattern is found in header,
+ * routine sets corresponding bit in auto_headers_mask.
+ * Returns -1 when header is reserved and it's bit is already set
+ * in auto_headers_mask; 0 otherwise,
+*/
+static int
+httpc_managed_headers_mask_update(uint8_t *auto_headers_mask,
+ const char *header)
+{
+ static struct {
+ const char *header_name;
+ uint32_t header_name_len;
+ } httpc_managed_headers[] = {
+ {"Connection", 10},
+ {"Keep-Alive", 10},
+ {"Content-Length", 14},
+ {"Accept", 6},
+ };
+ uint32_t httpc_managed_headers_cnt =
+ sizeof(httpc_managed_headers)/sizeof(httpc_managed_headers[0]);
+ assert(httpc_managed_headers_cnt <
+ sizeof(*auto_headers_mask) * CHAR_BIT);
+ for (uint32_t i = 0; i < httpc_managed_headers_cnt; i++) {
+ if (strncasecmp(header, httpc_managed_headers[i].header_name,
+ httpc_managed_headers[i].header_name_len) != 0)
+ continue;
+ int8_t bit_index = 1 << i;
+ if ((*auto_headers_mask & bit_index) != 0)
+ return -1;
+ *auto_headers_mask |= bit_index;
+ return 0;
+ }
+ return 0;
+}
+
int
httpc_set_header(struct httpc_request *req, const char *fmt, ...)
{
@@ -178,6 +179,11 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...)
const char *header = tt_vsprintf(fmt, ap);
va_end(ap);

+ if (httpc_managed_headers_mask_update(&req->auto_headers_mask,
+ header) != 0) {
+ /* Do not add extra header to list. */
+ return 0;
+ }
struct curl_slist *l = curl_slist_append(req->headers, header);
if (l == NULL) {
diag_set(OutOfMemory, strlen(header), "curl", "http header");
@@ -187,6 +193,47 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...)
return 0;
}

+int
+httpc_set_resource(struct httpc_request *req, const char *method,
+ const char *url)
+{
+ if (strcmp(method, "GET") == 0) {
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L);
+ } else if (strcmp(method, "HEAD") == 0) {
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_NOBODY, 1L);
+ } else if (strcmp(method, "POST") == 0 ||
+ strcmp(method, "PUT") == 0 ||
+ strcmp(method, "PATCH")) {
+ /*
+ * Set CURLOPT_POSTFIELDS to "" and CURLOPT_POSTFIELDSSIZE 0
+ * to avoid the read callback in any cases even if user
+ * forgot to call httpc_set_body() for POST request.
+ * @see https://curl.haxx.se/libcurl/c/CURLOPT_POSTFIELDS.html
+ */
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_POST, 1L);
+ 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)
+ return -1;
+ } else {
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
+ }
+
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
+
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_FOLLOWLOCATION, 1);
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_SSL_VERIFYPEER, 1);
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_WRITEFUNCTION,
+ curl_easy_write_cb);
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_HEADERFUNCTION,
+ curl_easy_header_cb);
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_NOPROGRESS, 1L);
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTP_VERSION,
+ CURL_HTTP_VERSION_1_1);
+ return 0;
+}
+
int
httpc_set_body(struct httpc_request *req, const char *body, size_t size)
{
diff --git a/src/httpc.h b/src/httpc.h
index dc709e6fa..8b400ae43 100644
--- a/src/httpc.h
+++ b/src/httpc.h
@@ -109,16 +109,23 @@ struct httpc_request {
struct region resp_headers;
/** buffer of body */
struct region resp_body;
+ /**
+ * A bitmask of the http request headers from among
+ * the headers that httpc can set automatically.
+ * This allows to prevent settings such headers twice.
+ * Read httpc_managed_headers_mask_update definition for
+ * more details.
+ */
+ uint8_t auto_headers_mask;
};

/**
* @brief Create a new HTTP request
- * @param ctx - reference to context
+ * @param env - HTTP client environment.
* @return a new HTTP request object
*/
struct httpc_request *
-httpc_request_new(struct httpc_env *env, const char *method,
- const char *url);
+httpc_request_new(struct httpc_env *env);

/**
* @brief Delete HTTP request
@@ -137,6 +144,17 @@ httpc_request_delete(struct httpc_request *req);
int
httpc_set_header(struct httpc_request *req, const char *fmt, ...);

+/**
+ * Set HTTP method and destination url, set service headers.
+ * @param req request
+ * @param method HTTP request method (GET, HEAD, POST, PUT...)
+ * @param url The network resource URL address string.
+ * @retval 0 for success, -1 otherwise
+ */
+int
+httpc_set_resource(struct httpc_request *req, const char *method,
+ const char *url);
+
/**
* Sets body of request
* @param req request
diff --git a/src/lua/httpc.c b/src/lua/httpc.c
index 5572b70e9..a3a311c64 100644
--- a/src/lua/httpc.c
+++ b/src/lua/httpc.c
@@ -145,12 +145,36 @@ luaT_httpc_request(lua_State *L)
if (ctx == NULL)
return luaL_error(L, "can't get httpc environment");

+ struct httpc_request *req = httpc_request_new(ctx);
+ if (req == NULL)
+ return luaT_error(L);
+
+ /*
+ * All user-defined headers must be set before calling
+ * other httpc module methods so that they take priority
+ * over the headers that httpc tries to set automatically.
+ */
+ lua_getfield(L, 5, "headers");
+ if (!lua_isnil(L, -1)) {
+ lua_pushnil(L);
+ while (lua_next(L, -2) != 0) {
+ if (httpc_set_header(req, "%s: %s",
+ lua_tostring(L, -2),
+ lua_tostring(L, -1)) < 0) {
+ httpc_request_delete(req);
+ return luaT_error(L);
+ }
+ lua_pop(L, 1);
+ }
+ }
+ lua_pop(L, 1);
+
const char *method = luaL_checkstring(L, 2);
const char *url = luaL_checkstring(L, 3);
-
- struct httpc_request *req = httpc_request_new(ctx, method, url);
- if (req == NULL)
+ if (httpc_set_resource(req, method, url) != 0) {
+ httpc_request_delete(req);
return luaT_error(L);
+ }

double timeout = TIMEOUT_INFINITY;
int max_header_name_length = HEADER_NAME_LEN;
@@ -171,21 +195,6 @@ luaT_httpc_request(lua_State *L)
return luaL_error(L, "fifth argument must be a table");
}

- lua_getfield(L, 5, "headers");
- if (!lua_isnil(L, -1)) {
- lua_pushnil(L);
- while (lua_next(L, -2) != 0) {
- if (httpc_set_header(req, "%s: %s",
- lua_tostring(L, -2),
- lua_tostring(L, -1)) < 0) {
- httpc_request_delete(req);
- return luaT_error(L);
- }
- lua_pop(L, 1);
- }
- }
- lua_pop(L, 1);
-
lua_getfield(L, 5, "ca_path");
if (!lua_isnil(L, -1))
httpc_set_ca_path(req, lua_tostring(L, -1));
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 10a3728f8..26119fa73 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -79,6 +79,15 @@ local function test_http_client(test, url, opts)
test:is(r.status, 200, 'request')
end

+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)
@@ -397,9 +406,11 @@ local function test_concurrent(test, url, opts)
end

function run_tests(test, sock_family, sock_addr)
- test:plan(9)
+ test:plan(10)
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.20.1

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 14 comments below.

On 25/02/2019 10:52, Kirill Shcherbatov wrote:
> I've reworked code a bit to set all user-defined headers before other httpc
> methods call to they have a priority.
> ================================================
> 
> 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.
> Was introduced service http c_managed_header_mask mask in which
> the bits corresponding to the http auto-manager headers, set 1
> when referenced. When the bit is set, the new value is not
> applied. In this way, we prefer user-defined value for all
> httpc-managed headers.
> 
> Closes #3955
> ---
> src/httpc.c | 123 +++++++++++++++++++++---------
> src/httpc.h | 24 +++++-
> src/lua/httpc.c | 45 ++++++-----
> test/app-tap/http_client.test.lua | 13 +++-
> 4 files changed, 145 insertions(+), 60 deletions(-)
> 
> diff --git a/src/httpc.c b/src/httpc.c
> index 950f8b32f..4685a712a 100644
> --- a/src/httpc.c
> +++ b/src/httpc.c
> @@ -95,8 +95,7 @@ httpc_env_destroy(struct httpc_env *ctx)
> }
> 
> struct httpc_request *
> -httpc_request_new(struct httpc_env *env, const char *method,
> - const char *url)
> +httpc_request_new(struct httpc_env *env)
> {
> struct httpc_request *req = mempool_alloc(&env->req_pool);
> if (req == NULL) {
> @@ -110,42 +109,7 @@ httpc_request_new(struct httpc_env *env, const char *method,
> region_create(&req->resp_body, &cord()->slabc);
> 
> if (curl_request_create(&req->curl_request) != 0)
> - return NULL;
> -
> - if (strcmp(method, "GET") == 0) {
> - curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L);
> - } else if (strcmp(method, "HEAD") == 0) {
> - curl_easy_setopt(req->curl_request.easy, CURLOPT_NOBODY, 1L);
> - } else if (strcmp(method, "POST") == 0 ||
> - strcmp(method, "PUT") == 0 ||
> - strcmp(method, "PATCH")) {
> - /*
> - * Set CURLOPT_POSTFIELDS to "" and CURLOPT_POSTFIELDSSIZE 0
> - * to avoid the read callback in any cases even if user
> - * forgot to call httpc_set_body() for POST request.
> - * @see https://curl.haxx.se/libcurl/c/CURLOPT_POSTFIELDS.html
> - */
> - curl_easy_setopt(req->curl_request.easy, CURLOPT_POST, 1L);
> - 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;
> - } else {
> - curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
> - }

1. The email is utterly corrupted - all the indentation is lost. Please,
do not copypaste patches into an email without proper preparations.

For example, if you want to resend a patch with some comments, then either

     - Use 'create_commits.sh', or raw 'git format-patch' to create a patch
       email. Then edit it manually right in the file. Then send that file
       using 'git send-email' with '--in-reply-to=<MessageID>' option.

     Or

     - Use 'git --no-pager show/diff/... > patch.txt' to dump diff or any
       other git output into a file as is, without any changes of
       indentation, and 'less' editor view. Depending on your text editor,
       you may need to change indentation width and type in that file.
       Then you can copypaste that file into a new email.

Below I put a normal version.

> diff --git a/src/httpc.c b/src/httpc.c
> index 950f8b32f..4685a712a 100644
> --- a/src/httpc.c
> +++ b/src/httpc.c
> @@ -110,42 +109,7 @@ httpc_request_new(struct httpc_env *env, const char *method,
>  	region_create(&req->resp_body, &cord()->slabc);
>  
>  	if (curl_request_create(&req->curl_request) != 0)
> -		return NULL;
> -
> -	if (strcmp(method, "GET") == 0) {
> -		curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L);
> -	} else if (strcmp(method, "HEAD") == 0) {
> -		curl_easy_setopt(req->curl_request.easy, CURLOPT_NOBODY, 1L);
> -	} else if (strcmp(method, "POST") == 0 ||
> -		   strcmp(method, "PUT") == 0 ||
> -		   strcmp(method, "PATCH")) {
> -		/*
> -		 * Set CURLOPT_POSTFIELDS to "" and CURLOPT_POSTFIELDSSIZE 0
> -		 * to avoid the read callback in any cases even if user
> -		 * forgot to call httpc_set_body() for POST request.
> -		 * @see https://curl.haxx.se/libcurl/c/CURLOPT_POSTFIELDS.html
> -		 */
> -		curl_easy_setopt(req->curl_request.easy, CURLOPT_POST, 1L);
> -		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;
> -	} else {
> -		curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
> -	}
> -
> -	curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
> -
> -	curl_easy_setopt(req->curl_request.easy, CURLOPT_FOLLOWLOCATION, 1);
> -	curl_easy_setopt(req->curl_request.easy, CURLOPT_SSL_VERIFYPEER, 1);
> -	curl_easy_setopt(req->curl_request.easy, CURLOPT_WRITEFUNCTION,
> -			 curl_easy_write_cb);
> -	curl_easy_setopt(req->curl_request.easy, CURLOPT_HEADERFUNCTION,
> -			 curl_easy_header_cb);
> -	curl_easy_setopt(req->curl_request.easy, CURLOPT_NOPROGRESS, 1L);
> -	curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTP_VERSION,
> -			 CURL_HTTP_VERSION_1_1);
> +		goto error;

2. In such a small function it is already clear, that you
do not need this single 'goto error'. Just process that error
here.

>  
>  	ibuf_create(&req->body, &cord()->slabc, 1);
>  
> @@ -170,6 +134,43 @@ httpc_request_delete(struct httpc_request *req)
>  	mempool_free(&req->env->req_pool, req);
>  }
>  
> +/**
> + * Update bitmask of the http request headers that httpc may set
> + * automatically. In case of reserved pattern is found in header,
> + * routine sets corresponding bit in auto_headers_mask.
> + * Returns -1 when header is reserved and it's bit is already set
> + * in auto_headers_mask; 0 otherwise,
> +*/

3. Use doxygen style.

> +static int
> +httpc_managed_headers_mask_update(uint8_t *auto_headers_mask,
> +				  const char *header)

4. I do not understand the name. Is it supposed to be a check?
Then, firstly, for checks we usually use '<object>_is/has_<property>'
naming template. But anyway it is not a check, because it changes
the mask. Come up with a better name, or use, for example, this:

     httpc_set_header_bit

> +{
> +	static struct {
> +		const char *header_name;
> +		uint32_t header_name_len;

5. The code below looks like Java. Please, rename to just 'name'
and 'len'.

> +	} httpc_managed_headers[] = {
> +		{"Connection",		10},
> +		{"Keep-Alive",		10},
> +		{"Content-Length",	14},
> +		{"Accept",		6},

6. I do not see any point in storing lengths here, because
anyway you can not use memcmp. Strncasecmp below will check
for terminating zero in both parameters anyway.

> +	};
> +	uint32_t httpc_managed_headers_cnt =
> +		sizeof(httpc_managed_headers)/sizeof(httpc_managed_headers[0]);

7. We have lengthof in trivia/utils.h.

> +	assert(httpc_managed_headers_cnt <
> +	       sizeof(*auto_headers_mask) * CHAR_BIT);
> +	for (uint32_t i = 0; i < httpc_managed_headers_cnt; i++) {

8. Please, do not use non standard types without necessity.
https://github.com/tarantool/tarantool/issues/2161

> +		if (strncasecmp(header, httpc_managed_headers[i].header_name,
> +				httpc_managed_headers[i].header_name_len) != 0)

9. As I said at least 2 times for one of previous issues, strncmp does
not work properly without additional checks. Your code will consider
'Acceptttttt' and 'Accept' to be equal. Please, fix that and add a test.

> +			continue;
> +		int8_t bit_index = 1 << i;
> +		if ((*auto_headers_mask & bit_index) != 0)
> +			return -1;
> +		*auto_headers_mask |= bit_index;
> +		return 0;
> +	}

10. I propose you to store const headers of httpc_managed_headers in a sorted
order. Sorted by the header names. Then, if during an iteration in the cycle
above you see, that strncasecmp() returned < 0, you can return without
further checks. I hope, that you understand why.

Another option is a binary search, but I think, that it is overkill here.

> +	return 0;
> +}
> +
>  int
>  httpc_set_header(struct httpc_request *req, const char *fmt, ...)
>  {
> @@ -187,6 +193,47 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...)
>  	return 0;
>  }
>  
> +int
> +httpc_set_resource(struct httpc_request *req, const char *method,
> +		   const char *url)

11. Firstly, it is a method of http_request, and should have
prefix 'http_request_'. But as I see, it is a common style
violation for this library, so lets turn the blind eye to it.

Secondly, personally I would like 'httpc_set_url', because it
looks shorter. And because 'resource' confused me at a first
glance.

Thirdly, please, move that function implementation below
httpc_request_new to reduce diff.

> +{
> +	if (strcmp(method, "GET") == 0) {
> +		curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L);
> +	} else if (strcmp(method, "HEAD") == 0) {
> +		curl_easy_setopt(req->curl_request.easy, CURLOPT_NOBODY, 1L);
> +	} else if (strcmp(method, "POST") == 0 ||
> +		   strcmp(method, "PUT") == 0 ||
> +		   strcmp(method, "PATCH")) {
> +		/*
> +		 * Set CURLOPT_POSTFIELDS to "" and CURLOPT_POSTFIELDSSIZE 0
> +		 * to avoid the read callback in any cases even if user
> +		 * forgot to call httpc_set_body() for POST request.
> +		 * @see https://curl.haxx.se/libcurl/c/CURLOPT_POSTFIELDS.html
> +		 */
> +		curl_easy_setopt(req->curl_request.easy, CURLOPT_POST, 1L);
> +		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)
> +			return -1;
> +	} else {
> +		curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
> +	}
> +
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
> +
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_FOLLOWLOCATION, 1);
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_SSL_VERIFYPEER, 1);
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_WRITEFUNCTION,
> +			 curl_easy_write_cb);
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_HEADERFUNCTION,
> +			 curl_easy_header_cb);
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_NOPROGRESS, 1L);
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTP_VERSION,
> +			 CURL_HTTP_VERSION_1_1);
> +	return 0;
> +}
> +
>  int
>  httpc_set_body(struct httpc_request *req, const char *body, size_t size)
>  {
> diff --git a/src/httpc.h b/src/httpc.h
> index dc709e6fa..8b400ae43 100644
> --- a/src/httpc.h
> +++ b/src/httpc.h
> @@ -109,16 +109,23 @@ struct httpc_request {
>  	struct region resp_headers;
>  	/** buffer of body */
>  	struct region resp_body;
> +	/**
> +	 * A bitmask of the http request headers from among
> +	 * the headers that httpc can set automatically.
> +	 * This allows to prevent settings such headers twice.
> +	 * Read httpc_managed_headers_mask_update definition for
> +	 * more details.

12. Do not put references 'for details' to some internal
functions. They likely to be changed, or renamed, or deleted,
and your reference will be useless. Explain here everything.

> +	 */
> +	uint8_t auto_headers_mask;
>  };
> @@ -137,6 +144,17 @@ httpc_request_delete(struct httpc_request *req);
>  int
>  httpc_set_header(struct httpc_request *req, const char *fmt, ...);
>  
> +/**
> + * Set  HTTP method and destination url, set service headers.
> + * @param req request
> + * @param method HTTP request method (GET, HEAD, POST, PUT...)
> + * @param url The network resource URL address string.
> + * @retval 0 for success, -1 otherwise

13.
- Please, start each @<option> description with a capital letter,
   and finish with a dot;

- Please, use separate @retval for different return values
   description.

- Do not put double whitespaces in your comments. Here and in other
   places too.

> + */
> +int
> +httpc_set_resource(struct httpc_request *req, const char *method,
> +		   const char *url);
> +
>  /**
>   * Sets body of request
>   * @param req request
> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
> index 10a3728f8..26119fa73 100755
> --- a/test/app-tap/http_client.test.lua
> +++ b/test/app-tap/http_client.test.lua
> @@ -79,6 +79,15 @@ local function test_http_client(test, url, opts)
>      test:is(r.status, 200, 'request')
>  end
>  
> +local function test_http_client_headers_redefine(test, url, opts)

14. Setting a new test out, please, write a comment about what this
test tests, and put a github issue number in it. Conforming our
test comments style.

> +    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
> +

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

[Kirill Shcherbatov]

Hi! Thank you for review.

> 1. The email is utterly corrupted - all the indentation is lost. Please,
> do not copypaste patches into an email without proper preparations.
Ok.

> 2. In such a small function it is already clear, that you
> do not need this single 'goto error'. Just process that error
> here.
Done.

> 3. Use doxygen style.> 4. I do not understand the name. Is it supposed to be a check?
> Then, firstly, for checks we usually use '<object>_is/has_<property>'
> naming template. But anyway it is not a check, because it changes
> the mask. Come up with a better name, or use, for example, this:
> 
>      httpc_set_header_bit
/**
 * Update bitmask of the http request headers that httpc may set
 * automatically. In case of reserved pattern is found in header,
 * routine sets corresponding bit in auto_headers_mask.
 * Returns -1 when header is reserved and it's bit is already set
 * in auto_headers_mask; 0 otherwise,
 * @param auto_headers_mask The bitmask of httpc-auto-managed
 *                          headers to pointer.
 * @param header The HTTP header string.
 * @retval 0 When specified header is not auto-managed or when
 *           corresponding bit was not set in auto_headers_mask.
 * @retval -1 otherwise.
*/
static int
httpc_set_header_bit(uint8_t *auto_headers_mask, const char *header)


> 5. The code below looks like Java. Please, rename to just 'name'
> and 'len'.> 6. I do not see any point in storing lengths here, because
> anyway you can not use memcmp. Strncasecmp below will check
> for terminating zero in both parameters anyway.
It is not so, because we must compare just only prefix of specified length.
For more details, show 9th comment.

> 7. We have lengthof in trivia/utils.h.
        /*
	 * The sequence of managed headers must be sorted to
	 * stop scan when strcasecmp < 0. The header is expected
	 * to be formated with "%s: %s" pattern, so direct size
	 * verification is redundant.
	 */
	struct {
		const char *name;
		int len;
	} managed_headers[] = {
		{"Accept: ", sizeof("Accept: ") - 1},
		{"Connection: ", sizeof("Connection: ") - 1},
		{"Content-Length: ", sizeof("Content-Length: ") - 1},
		{"Keep-Alive: ", sizeof("Keep-Alive: ") - 1},
	};
	

> 8. Please, do not use non standard types without necessity.
> https://github.com/tarantool/tarantool/issues/2161
for (int i = 0; i < managed_headers_cnt; i++) {

> 9. As I said at least 2 times for one of previous issues, strncmp does
> not work properly without additional checks. Your code will consider
> 'Acceptttttt' and 'Accept' to be equal. Please, fix that and add a test.
It is not so here, because header is a string that have strong format "%s: %s"
I've modified managed_headers array below to use "%s: " prefix.
Here It is not a bug, but feature =)

> 10. I propose you to store const headers of httpc_managed_headers in a sorted
> order. Sorted by the header names. Then, if during an iteration in the cycle
> above you see, that strncasecmp() returned < 0, you can return without
> further checks. I hope, that you understand why.
> 
> Another option is a binary search, but I think, that it is overkill here.
Implemented.

> 11. Firstly, it is a method of http_request, and should have
> prefix 'http_request_'. But as I see, it is a common style
> violation for this library, so lets turn the blind eye to it.
> 
> Secondly, personally I would like 'httpc_set_url', because it
> looks shorter. And because 'resource' confused me at a first
> glance.
> 
> Thirdly, please, move that function implementation below
> httpc_request_new to reduce diff.
Done.

> 12. Do not put references 'for details' to some internal
> functions. They likely to be changed, or renamed, or deleted,
> and your reference will be useless. Explain here everything.
	/**
	 * A bitmask of the http request headers from among
	 * the headers that httpc can set automatically.
	 * Each bit represents some auto-managed HTTP header.
	 * This allows to prevent set them twice.
	 */
	uint8_t auto_headers_mask;

> 13.
> - Please, start each @<option> description with a capital letter,
>    and finish with a dot;
> 
> - Please, use separate @retval for different return values
>    description.
> 
> - Do not put double whitespaces in your comments. Here and in other
>    places too.
(I just copied the template of the next comment, they are all like that.)
/**
 * Set HTTP method and destination url, set service headers.
 * @param req HTTP request object to update.
 * @param method HTTP request method (GET, HEAD, POST, PUT...).
 * @param url The network resource URL address string.
 * @retval 0 On success.
 * @retval -1 Otherwise.
 */
int
httpc_set_url(struct httpc_request *req, const char *method, const char *url);

> 
>> + */
>> +int
>> +httpc_set_resource(struct httpc_request *req, const char *method,
>> +		   const char *url);
>> +
>>  /**
>>   * Sets body of request
>>   * @param req request
>> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
>> index 10a3728f8..26119fa73 100755
>> --- a/test/app-tap/http_client.test.lua
>> +++ b/test/app-tap/http_client.test.lua
>> @@ -79,6 +79,15 @@ local function test_http_client(test, url, opts)
>>      test:is(r.status, 200, 'request')
>>  end
>>  
>> +local function test_http_client_headers_redefine(test, url, opts)
> 
> 14. Setting a new test out, please, write a comment about what this
> test tests, and put a github issue number in it. Conforming our
> test comments style.
--
-- gh-3955: fix httpc auto-managed headers
--
local function test_http_client_headers_redefine(test, url, opts)

============================================

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.
Was introduced service http c_managed_header_mask mask in which
the bits corresponding to the http auto-manager headers, set 1
when referenced. When the bit is set, the new value is not
applied. In this way, we prefer user-defined value for all
httpc-managed headers.

Closes #3955
---
 src/httpc.c                       | 80 ++++++++++++++++++++++++++-----
 src/httpc.h                       | 23 +++++++--
 src/lua/httpc.c                   | 45 ++++++++++-------
 test/app-tap/http_client.test.lua | 16 ++++++-
 4 files changed, 131 insertions(+), 33 deletions(-)

diff --git a/src/httpc.c b/src/httpc.c
index 950f8b32f..96740f950 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -33,6 +33,7 @@
 
 #include <assert.h>
 #include <curl/curl.h>
+#include <trivia/util.h>
 
 #include "fiber.h"
 #include "errinj.h"
@@ -95,8 +96,7 @@ httpc_env_destroy(struct httpc_env *ctx)
 }
 
 struct httpc_request *
-httpc_request_new(struct httpc_env *env, const char *method,
-		  const char *url)
+httpc_request_new(struct httpc_env *env)
 {
 	struct httpc_request *req = mempool_alloc(&env->req_pool);
 	if (req == NULL) {
@@ -109,9 +109,19 @@ httpc_request_new(struct httpc_env *env, const char *method,
 	region_create(&req->resp_headers, &cord()->slabc);
 	region_create(&req->resp_body, &cord()->slabc);
 
-	if (curl_request_create(&req->curl_request) != 0)
+	if (curl_request_create(&req->curl_request) != 0) {
+		mempool_free(&env->req_pool, req);
 		return NULL;
+	}
+
+	ibuf_create(&req->body, &cord()->slabc, 1);
+
+	return req;
+}
 
+int
+httpc_set_url(struct httpc_request *req, const char *method, const char *url)
+{
 	if (strcmp(method, "GET") == 0) {
 		curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L);
 	} else if (strcmp(method, "HEAD") == 0) {
@@ -130,7 +140,7 @@ httpc_request_new(struct httpc_env *env, const char *method,
 		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;
+			return -1;
 	} else {
 		curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
 	}
@@ -146,13 +156,7 @@ httpc_request_new(struct httpc_env *env, const char *method,
 	curl_easy_setopt(req->curl_request.easy, CURLOPT_NOPROGRESS, 1L);
 	curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTP_VERSION,
 			 CURL_HTTP_VERSION_1_1);
-
-	ibuf_create(&req->body, &cord()->slabc, 1);
-
-	return req;
-error:
-	mempool_free(&env->req_pool, req);
-	return NULL;
+	return 0;
 }
 
 void
@@ -170,6 +174,56 @@ httpc_request_delete(struct httpc_request *req)
 	mempool_free(&req->env->req_pool, req);
 }
 
+/**
+ * Update bitmask of the http request headers that httpc may set
+ * automatically. In case of reserved pattern is found in header,
+ * routine sets corresponding bit in auto_headers_mask.
+ * Returns -1 when header is reserved and it's bit is already set
+ * in auto_headers_mask; 0 otherwise,
+ * @param auto_headers_mask The bitmask of httpc-auto-managed
+ *                          headers to pointer.
+ * @param header The HTTP header string.
+ * @retval 0 When specified header is not auto-managed or when
+ *           corresponding bit was not set in auto_headers_mask.
+ * @retval -1 otherwise.
+*/
+static int
+httpc_set_header_bit(uint8_t *auto_headers_mask, const char *header)
+{
+	/*
+	 * The sequence of managed headers must be sorted to
+	 * stop scan when strcasecmp < 0. The header is expected
+	 * to be formated with "%s: %s" pattern, so direct size
+	 * verification is redundant.
+	 */
+	struct {
+		const char *name;
+		int len;
+	} managed_headers[] = {
+		{"Accept: ", sizeof("Accept: ") - 1},
+		{"Connection: ", sizeof("Connection: ") - 1},
+		{"Content-Length: ", sizeof("Content-Length: ") - 1},
+		{"Keep-Alive: ", sizeof("Keep-Alive: ") - 1},
+	};
+	int managed_headers_cnt = lengthof(managed_headers);
+	assert(managed_headers_cnt <
+	       (int)sizeof(*auto_headers_mask) * CHAR_BIT);
+	for (int i = 0; i < managed_headers_cnt; i++) {
+		int rc = strncasecmp(header, managed_headers[i].name,
+				     managed_headers[i].len);
+		if (rc > 0)
+			continue;
+		if (rc < 0)
+			return 0;
+		int8_t bit_index = 1 << i;
+		if ((*auto_headers_mask & bit_index) != 0)
+			return -1;
+		*auto_headers_mask |= bit_index;
+		return 0;
+	}
+	return 0;
+}
+
 int
 httpc_set_header(struct httpc_request *req, const char *fmt, ...)
 {
@@ -178,6 +232,10 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...)
 	const char *header = tt_vsprintf(fmt, ap);
 	va_end(ap);
 
+	if (httpc_set_header_bit(&req->auto_headers_mask, header) != 0) {
+		/* Do not add extra header to list. */
+		return 0;
+	}
 	struct curl_slist *l = curl_slist_append(req->headers, header);
 	if (l == NULL) {
 		diag_set(OutOfMemory, strlen(header), "curl", "http header");
diff --git a/src/httpc.h b/src/httpc.h
index dc709e6fa..dc18ffb69 100644
--- a/src/httpc.h
+++ b/src/httpc.h
@@ -109,16 +109,22 @@ struct httpc_request {
 	struct region resp_headers;
 	/** buffer of body */
 	struct region resp_body;
+	/**
+	 * A bitmask of the http request headers from among
+	 * the headers that httpc can set automatically.
+	 * Each bit represents some auto-managed HTTP header.
+	 * This allows to prevent set them twice.
+	 */
+	uint8_t auto_headers_mask;
 };
 
 /**
  * @brief Create a new HTTP request
- * @param ctx - reference to context
+ * @param env - HTTP client environment.
  * @return a new HTTP request object
  */
 struct httpc_request *
-httpc_request_new(struct httpc_env *env, const char *method,
-		  const char *url);
+httpc_request_new(struct httpc_env *env);
 
 /**
  * @brief Delete HTTP request
@@ -137,6 +143,17 @@ httpc_request_delete(struct httpc_request *req);
 int
 httpc_set_header(struct httpc_request *req, const char *fmt, ...);
 
+/**
+ * Set HTTP method and destination url, set service headers.
+ * @param req HTTP request object to update.
+ * @param method HTTP request method (GET, HEAD, POST, PUT...).
+ * @param url The network resource URL address string.
+ * @retval 0 On success.
+ * @retval -1 Otherwise.
+ */
+int
+httpc_set_url(struct httpc_request *req, const char *method, const char *url);
+
 /**
  * Sets body of request
  * @param req request
diff --git a/src/lua/httpc.c b/src/lua/httpc.c
index 5572b70e9..d743eeb49 100644
--- a/src/lua/httpc.c
+++ b/src/lua/httpc.c
@@ -145,12 +145,36 @@ luaT_httpc_request(lua_State *L)
 	if (ctx == NULL)
 		return luaL_error(L, "can't get httpc environment");
 
+	struct httpc_request *req = httpc_request_new(ctx);
+	if (req == NULL)
+		return luaT_error(L);
+
+	/*
+	 * All user-defined headers must be set before calling
+	 * other httpc module methods so that they take priority
+	 * over the headers that httpc tries to set automatically.
+	 */
+	lua_getfield(L, 5, "headers");
+	if (!lua_isnil(L, -1)) {
+		lua_pushnil(L);
+		while (lua_next(L, -2) != 0) {
+			if (httpc_set_header(req, "%s: %s",
+					     lua_tostring(L, -2),
+					     lua_tostring(L, -1)) < 0) {
+				httpc_request_delete(req);
+				return luaT_error(L);
+			}
+			lua_pop(L, 1);
+		}
+	}
+	lua_pop(L, 1);
+
 	const char *method = luaL_checkstring(L, 2);
 	const char *url  = luaL_checkstring(L, 3);
-
-	struct httpc_request *req = httpc_request_new(ctx, method, url);
-	if (req == NULL)
+	if (httpc_set_url(req, method, url) != 0) {
+		httpc_request_delete(req);
 		return luaT_error(L);
+	}
 
 	double timeout = TIMEOUT_INFINITY;
 	int max_header_name_length = HEADER_NAME_LEN;
@@ -171,21 +195,6 @@ luaT_httpc_request(lua_State *L)
 		return luaL_error(L, "fifth argument must be a table");
 	}
 
-	lua_getfield(L, 5, "headers");
-	if (!lua_isnil(L, -1)) {
-		lua_pushnil(L);
-		while (lua_next(L, -2) != 0) {
-			if (httpc_set_header(req, "%s: %s",
-					     lua_tostring(L, -2),
-					     lua_tostring(L, -1)) < 0) {
-				httpc_request_delete(req);
-				return luaT_error(L);
-			}
-			lua_pop(L, 1);
-		}
-	}
-	lua_pop(L, 1);
-
 	lua_getfield(L, 5, "ca_path");
 	if (!lua_isnil(L, -1))
 		httpc_set_ca_path(req, lua_tostring(L, -1));
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 10a3728f8..b8573a572 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -79,6 +79,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)
@@ -397,9 +409,11 @@ local function test_concurrent(test, url, opts)
 end
 
 function run_tests(test, sock_family, sock_addr)
-    test:plan(9)
+    test:plan(10)
     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.20.1

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

[Vladimir Davydov]

On Mon, Feb 25, 2019 at 06:21:31PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/httpc.c b/src/httpc.c
> index 950f8b32f..96740f950 100644
> --- a/src/httpc.c
> +++ b/src/httpc.c
> @@ -170,6 +174,56 @@ httpc_request_delete(struct httpc_request *req)
>  	mempool_free(&req->env->req_pool, req);
>  }
>  
> +/**
> + * Update bitmask of the http request headers that httpc may set
> + * automatically. In case of reserved pattern is found in header,
> + * routine sets corresponding bit in auto_headers_mask.
> + * Returns -1 when header is reserved and it's bit is already set
> + * in auto_headers_mask; 0 otherwise,
> + * @param auto_headers_mask The bitmask of httpc-auto-managed
> + *                          headers to pointer.
> + * @param header The HTTP header string.
> + * @retval 0 When specified header is not auto-managed or when
> + *           corresponding bit was not set in auto_headers_mask.
> + * @retval -1 otherwise.
> +*/
> +static int
> +httpc_set_header_bit(uint8_t *auto_headers_mask, const char *header)
> +{
> +	/*
> +	 * The sequence of managed headers must be sorted to
> +	 * stop scan when strcasecmp < 0. The header is expected
> +	 * to be formated with "%s: %s" pattern, so direct size
> +	 * verification is redundant.
> +	 */
> +	struct {
> +		const char *name;
> +		int len;
> +	} managed_headers[] = {
> +		{"Accept: ", sizeof("Accept: ") - 1},
> +		{"Connection: ", sizeof("Connection: ") - 1},
> +		{"Content-Length: ", sizeof("Content-Length: ") - 1},
> +		{"Keep-Alive: ", sizeof("Keep-Alive: ") - 1},

 - A space after a colon (:) isn't mandatory according to the http
   protocol, e.g. "content-length:10" is a valid header.

 - I don't quite like string literal duplication. Can we do without it?
   Use strlen, may be? The compiler should resolve it at compile time
   AFAIK.

 - Checking "content-length" doesn't make much sense IMO. No-one in
   their right mind will overwrite it after setting a body. We only
   need to handle "accept", "connection", "keep-alive".

> +	};
> +	int managed_headers_cnt = lengthof(managed_headers);
> +	assert(managed_headers_cnt <
> +	       (int)sizeof(*auto_headers_mask) * CHAR_BIT);
> +	for (int i = 0; i < managed_headers_cnt; i++) {
> +		int rc = strncasecmp(header, managed_headers[i].name,
> +				     managed_headers[i].len);
> +		if (rc > 0)
> +			continue;
> +		if (rc < 0)
> +			return 0;
> +		int8_t bit_index = 1 << i;
> +		if ((*auto_headers_mask & bit_index) != 0)
> +			return -1;
> +		*auto_headers_mask |= bit_index;
> +		return 0;
> +	}
> +	return 0;
> +}
> +
>  int
>  httpc_set_header(struct httpc_request *req, const char *fmt, ...)
>  {
> @@ -178,6 +232,10 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...)
>  	const char *header = tt_vsprintf(fmt, ap);
>  	va_end(ap);
>  
> +	if (httpc_set_header_bit(&req->auto_headers_mask, header) != 0) {
> +		/* Do not add extra header to list. */
> +		return 0;
> +	}
>  	struct curl_slist *l = curl_slist_append(req->headers, header);
>  	if (l == NULL) {
>  		diag_set(OutOfMemory, strlen(header), "curl", "http header");
> diff --git a/src/httpc.h b/src/httpc.h
> index dc709e6fa..dc18ffb69 100644
> --- a/src/httpc.h
> +++ b/src/httpc.h
> @@ -109,16 +109,22 @@ struct httpc_request {
>  	struct region resp_headers;
>  	/** buffer of body */
>  	struct region resp_body;
> +	/**
> +	 * A bitmask of the http request headers from among
> +	 * the headers that httpc can set automatically.
> +	 * Each bit represents some auto-managed HTTP header.
> +	 * This allows to prevent set them twice.
> +	 */
> +	uint8_t auto_headers_mask;
>  };
>  
>  /**
>   * @brief Create a new HTTP request
> - * @param ctx - reference to context
> + * @param env - HTTP client environment.
>   * @return a new HTTP request object
>   */
>  struct httpc_request *
> -httpc_request_new(struct httpc_env *env, const char *method,
> -		  const char *url);
> +httpc_request_new(struct httpc_env *env);
>  
>  /**
>   * @brief Delete HTTP request
> @@ -137,6 +143,17 @@ httpc_request_delete(struct httpc_request *req);
>  int
>  httpc_set_header(struct httpc_request *req, const char *fmt, ...);
>  
> +/**
> + * Set HTTP method and destination url, set service headers.
> + * @param req HTTP request object to update.
> + * @param method HTTP request method (GET, HEAD, POST, PUT...).
> + * @param url The network resource URL address string.
> + * @retval 0 On success.
> + * @retval -1 Otherwise.
> + */
> +int
> +httpc_set_url(struct httpc_request *req, const char *method, const char *url);
> +
>  /**
>   * Sets body of request
>   * @param req request
> diff --git a/src/lua/httpc.c b/src/lua/httpc.c
> index 5572b70e9..d743eeb49 100644
> --- a/src/lua/httpc.c
> +++ b/src/lua/httpc.c
> @@ -145,12 +145,36 @@ luaT_httpc_request(lua_State *L)
>  	if (ctx == NULL)
>  		return luaL_error(L, "can't get httpc environment");
>  
> +	struct httpc_request *req = httpc_request_new(ctx);
> +	if (req == NULL)
> +		return luaT_error(L);
> +
> +	/*
> +	 * All user-defined headers must be set before calling
> +	 * other httpc module methods so that they take priority
> +	 * over the headers that httpc tries to set automatically.
> +	 */
> +	lua_getfield(L, 5, "headers");
> +	if (!lua_isnil(L, -1)) {
> +		lua_pushnil(L);
> +		while (lua_next(L, -2) != 0) {
> +			if (httpc_set_header(req, "%s: %s",
> +					     lua_tostring(L, -2),
> +					     lua_tostring(L, -1)) < 0) {
> +				httpc_request_delete(req);
> +				return luaT_error(L);
> +			}
> +			lua_pop(L, 1);
> +		}
> +	}
> +	lua_pop(L, 1);
> +

I don't like how httpc internal API now looks like: one should set
headers before the method/url and keepalive options, otherwise he/she
risks loosing them. And there isn't a word about this idiosyncrasy
anywhere in the comments.

Also, splitting request construction into allocation (_new) and
method/url initialization (_set_url) looks ugly to me, because no
request makes sense without them.

May be, we could set default headers in httpc_execute instead?

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

[Kirill Shcherbatov]

>> +	/*
>> +	 * The sequence of managed headers must be sorted to
>> +	 * stop scan when strcasecmp < 0. The header is expected
>> +	 * to be formated with "%s: %s" pattern, so direct size
>> +	 * verification is redundant.
>> +	 */
>> +	struct {
>> +		const char *name;
>> +		int len;
>> +	} managed_headers[] = {
>> +		{"Accept: ", sizeof("Accept: ") - 1},
>> +		{"Connection: ", sizeof("Connection: ") - 1},
>> +		{"Content-Length: ", sizeof("Content-Length: ") - 1},
>> +		{"Keep-Alive: ", sizeof("Keep-Alive: ") - 1},
> 
>  - A space after a colon (:) isn't mandatory according to the http
>    protocol, e.g. "content-length:10" is a valid header.
All headers are formated by server earlier, so this is acceptable:
	if (httpc_set_header(req, "%s: %s",
					     lua_tostring(L, -2),
					     lua_tostring(L, -1)) < 0) {
				httpc_request_delete(req);
				return luaT_error(L);
			}

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch. Please,
apply the diff below and then LGTM.

diff --git a/src/httpc.c b/src/httpc.c
index 96740f950..060cacb48 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -113,9 +113,7 @@ httpc_request_new(struct httpc_env *env)
  		mempool_free(&env->req_pool, req);
  		return NULL;
  	}
-
  	ibuf_create(&req->body, &cord()->slabc, 1);
-
  	return req;
  }
  
@@ -178,14 +176,13 @@ httpc_request_delete(struct httpc_request *req)
   * Update bitmask of the http request headers that httpc may set
   * automatically. In case of reserved pattern is found in header,
   * routine sets corresponding bit in auto_headers_mask.
- * Returns -1 when header is reserved and it's bit is already set
- * in auto_headers_mask; 0 otherwise,
- * @param auto_headers_mask The bitmask of httpc-auto-managed
- *                          headers to pointer.
- * @param header The HTTP header string.
- * @retval 0 When specified header is not auto-managed or when
- *           corresponding bit was not set in auto_headers_mask.
- * @retval -1 otherwise.
+ * @param auto_headers_mask A bitmask of httpc-auto-managed
+ *        headers to pointer.
+ * @param header A HTTP header string.
+ *
+ * @retval 0 Specified header is not auto-managed or
+ *        corresponding bit was not set in auto_headers_mask.
+ * @retval -1 Otherwise.
  */
  static int
  httpc_set_header_bit(uint8_t *auto_headers_mask, const char *header)
@@ -196,7 +193,7 @@ httpc_set_header_bit(uint8_t *auto_headers_mask, const char *header)
  	 * to be formated with "%s: %s" pattern, so direct size
  	 * verification is redundant.
  	 */
-	struct {
+	static struct {
  		const char *name;
  		int len;
  	} managed_headers[] = {
diff --git a/src/httpc.h b/src/httpc.h
index dc18ffb69..361af57e1 100644
--- a/src/httpc.h
+++ b/src/httpc.h
@@ -120,7 +120,7 @@ struct httpc_request {
  
  /**
   * @brief Create a new HTTP request
- * @param env - HTTP client environment.
+ * @param env HTTP client environment.
   * @return a new HTTP request object
   */
  struct httpc_request *

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

[Vladimir Davydov]

On Thu, Mar 07, 2019 at 05:07:04PM +0300, Kirill Shcherbatov wrote:
> >> +	/*
> >> +	 * The sequence of managed headers must be sorted to
> >> +	 * stop scan when strcasecmp < 0. The header is expected
> >> +	 * to be formated with "%s: %s" pattern, so direct size
> >> +	 * verification is redundant.
> >> +	 */
> >> +	struct {
> >> +		const char *name;
> >> +		int len;
> >> +	} managed_headers[] = {
> >> +		{"Accept: ", sizeof("Accept: ") - 1},
> >> +		{"Connection: ", sizeof("Connection: ") - 1},
> >> +		{"Content-Length: ", sizeof("Content-Length: ") - 1},
> >> +		{"Keep-Alive: ", sizeof("Keep-Alive: ") - 1},
> > 
> >  - A space after a colon (:) isn't mandatory according to the http
> >    protocol, e.g. "content-length:10" is a valid header.
> All headers are formated by server earlier, so this is acceptable:
> 	if (httpc_set_header(req, "%s: %s",
> 					     lua_tostring(L, -2),
> 					     lua_tostring(L, -1)) < 0) {
> 				httpc_request_delete(req);
> 				return luaT_error(L);
> 			}

Yeah, okay, but looks kinda fragile if you consider the httpc module on
its own, without looking at the Lua binding code. So please change.

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

[Kirill Shcherbatov]

>  - A space after a colon (:) isn't mandatory according to the http
>    protocol, e.g. "content-length:10" is a valid header.
Done.
>  - I don't quite like string literal duplication. Can we do without it?
>    Use strlen, may be? The compiler should resolve it at compile time
>    AFAIK.
We need to compare strlen() characters, not compare whole strings; I don't
like to calculate it each time. As we disscused, let's keep it
> 
>  - Checking "content-length" doesn't make much sense IMO. No-one in
>    their right mind will overwrite it after setting a body. We only
>    need to handle "accept", "connection", "keep-alive".
Done.

> I don't like how httpc internal API now looks like: one should set
> headers before the method/url and keepalive options, otherwise he/she
> risks loosing them. And there isn't a word about this idiosyncrasy
> anywhere in the comments.
> 
> Also, splitting request construction into allocation (_new) and
> method/url initialization (_set_url) looks ugly to me, because no
> request makes sense without them.
> 
> May be, we could set default headers in httpc_execute instead?
No, when those headers are set, they use specific context: like size, idle etx.

I've also applied Vlad's diff (tnx to Vlad). Let's ask Kirill to merge it?)

======================================================

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.
Was introduced service http c_managed_header_mask mask in which
the bits corresponding to the http auto-manager headers, set 1
when referenced. When the bit is set, the new value is not
applied. In this way, we prefer user-defined value for all
httpc-managed headers.

Closes #3955
---
 src/httpc.c                       | 76 ++++++++++++++++++++++++++-----
 src/httpc.h                       | 23 ++++++++--
 src/lua/httpc.c                   | 55 ++++++++++++----------
 test/app-tap/http_client.test.lua | 16 ++++++-
 4 files changed, 132 insertions(+), 38 deletions(-)

diff --git a/src/httpc.c b/src/httpc.c
index 950f8b32f..ab745e26b 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -33,6 +33,7 @@
 
 #include <assert.h>
 #include <curl/curl.h>
+#include <trivia/util.h>
 
 #include "fiber.h"
 #include "errinj.h"
@@ -95,8 +96,7 @@ httpc_env_destroy(struct httpc_env *ctx)
 }
 
 struct httpc_request *
-httpc_request_new(struct httpc_env *env, const char *method,
-		  const char *url)
+httpc_request_new(struct httpc_env *env)
 {
 	struct httpc_request *req = mempool_alloc(&env->req_pool);
 	if (req == NULL) {
@@ -109,9 +109,17 @@ httpc_request_new(struct httpc_env *env, const char *method,
 	region_create(&req->resp_headers, &cord()->slabc);
 	region_create(&req->resp_body, &cord()->slabc);
 
-	if (curl_request_create(&req->curl_request) != 0)
+	if (curl_request_create(&req->curl_request) != 0) {
+		mempool_free(&env->req_pool, req);
 		return NULL;
+	}
+	ibuf_create(&req->body, &cord()->slabc, 1);
+	return req;
+}
 
+int
+httpc_set_url(struct httpc_request *req, const char *method, const char *url)
+{
 	if (strcmp(method, "GET") == 0) {
 		curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L);
 	} else if (strcmp(method, "HEAD") == 0) {
@@ -130,7 +138,7 @@ httpc_request_new(struct httpc_env *env, const char *method,
 		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;
+			return -1;
 	} else {
 		curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
 	}
@@ -146,13 +154,7 @@ httpc_request_new(struct httpc_env *env, const char *method,
 	curl_easy_setopt(req->curl_request.easy, CURLOPT_NOPROGRESS, 1L);
 	curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTP_VERSION,
 			 CURL_HTTP_VERSION_1_1);
-
-	ibuf_create(&req->body, &cord()->slabc, 1);
-
-	return req;
-error:
-	mempool_free(&env->req_pool, req);
-	return NULL;
+	return 0;
 }
 
 void
@@ -170,6 +172,54 @@ httpc_request_delete(struct httpc_request *req)
 	mempool_free(&req->env->req_pool, req);
 }
 
+/**
+ * Update bitmask of the http request headers that httpc may set
+ * automatically. In case of reserved pattern is found in header,
+ * routine sets corresponding bit in auto_headers_mask.
+ * @param auto_headers_mask A bitmask of httpc-auto-managed
+ *        headers to pointer.
+ * @param header A HTTP header string.
+ *
+ * @retval 0 Specified header is not auto-managed or
+ *        corresponding bit was not set in auto_headers_mask.
+ * @retval -1 Otherwise.
+*/
+static int
+httpc_set_header_bit(uint8_t *auto_headers_mask, const char *header)
+{
+	/*
+	 * The sequence of managed headers must be sorted to
+	 * stop scan when strcasecmp < 0. The header is expected
+	 * to be formated with "%s: %s" pattern, so direct size
+	 * verification is redundant.
+	 */
+	static struct {
+		const char *name;
+		int len;
+	} managed_headers[] = {
+		{"Accept:",     sizeof("Accept:") - 1    },
+		{"Connection:", sizeof("Connection:") - 1},
+		{"Keep-Alive:", sizeof("Keep-Alive:") - 1},
+	};
+	int managed_headers_cnt = lengthof(managed_headers);
+	assert(managed_headers_cnt <
+	       (int)sizeof(*auto_headers_mask) * CHAR_BIT);
+	for (int i = 0; i < managed_headers_cnt; i++) {
+		int rc = strncasecmp(header, managed_headers[i].name,
+				     managed_headers[i].len);
+		if (rc > 0)
+			continue;
+		if (rc < 0)
+			return 0;
+		int8_t bit_index = 1 << i;
+		if ((*auto_headers_mask & bit_index) != 0)
+			return -1;
+		*auto_headers_mask |= bit_index;
+		return 0;
+	}
+	return 0;
+}
+
 int
 httpc_set_header(struct httpc_request *req, const char *fmt, ...)
 {
@@ -178,6 +228,10 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...)
 	const char *header = tt_vsprintf(fmt, ap);
 	va_end(ap);
 
+	if (httpc_set_header_bit(&req->auto_headers_mask, header) != 0) {
+		/* Do not add extra header to list. */
+		return 0;
+	}
 	struct curl_slist *l = curl_slist_append(req->headers, header);
 	if (l == NULL) {
 		diag_set(OutOfMemory, strlen(header), "curl", "http header");
diff --git a/src/httpc.h b/src/httpc.h
index dc709e6fa..361af57e1 100644
--- a/src/httpc.h
+++ b/src/httpc.h
@@ -109,16 +109,22 @@ struct httpc_request {
 	struct region resp_headers;
 	/** buffer of body */
 	struct region resp_body;
+	/**
+	 * A bitmask of the http request headers from among
+	 * the headers that httpc can set automatically.
+	 * Each bit represents some auto-managed HTTP header.
+	 * This allows to prevent set them twice.
+	 */
+	uint8_t auto_headers_mask;
 };
 
 /**
  * @brief Create a new HTTP request
- * @param ctx - reference to context
+ * @param env HTTP client environment.
  * @return a new HTTP request object
  */
 struct httpc_request *
-httpc_request_new(struct httpc_env *env, const char *method,
-		  const char *url);
+httpc_request_new(struct httpc_env *env);
 
 /**
  * @brief Delete HTTP request
@@ -137,6 +143,17 @@ httpc_request_delete(struct httpc_request *req);
 int
 httpc_set_header(struct httpc_request *req, const char *fmt, ...);
 
+/**
+ * Set HTTP method and destination url, set service headers.
+ * @param req HTTP request object to update.
+ * @param method HTTP request method (GET, HEAD, POST, PUT...).
+ * @param url The network resource URL address string.
+ * @retval 0 On success.
+ * @retval -1 Otherwise.
+ */
+int
+httpc_set_url(struct httpc_request *req, const char *method, const char *url);
+
 /**
  * Sets body of request
  * @param req request
diff --git a/src/lua/httpc.c b/src/lua/httpc.c
index 0006efdf3..4449b2854 100644
--- a/src/lua/httpc.c
+++ b/src/lua/httpc.c
@@ -145,32 +145,15 @@ luaT_httpc_request(lua_State *L)
 	if (ctx == NULL)
 		return luaL_error(L, "can't get httpc environment");
 
-	const char *method = luaL_checkstring(L, 2);
-	const char *url  = luaL_checkstring(L, 3);
-
-	struct httpc_request *req = httpc_request_new(ctx, method, url);
+	struct httpc_request *req = httpc_request_new(ctx);
 	if (req == NULL)
 		return luaT_error(L);
 
-	double timeout = TIMEOUT_INFINITY;
-	int max_header_name_length = HEADER_NAME_LEN;
-	if (lua_isstring(L, 4)) {
-		size_t len = 0;
-		const char *body = lua_tolstring(L, 4, &len);
-		if (len > 0 && httpc_set_body(req, body, len) != 0) {
-			httpc_request_delete(req);
-			return luaT_error(L);
-		}
-	} else if (!lua_isnil(L, 4)) {
-		httpc_request_delete(req);
-		return luaL_error(L, "fourth argument must be a string");
-	}
-
-	if (!lua_istable(L, 5)) {
-		httpc_request_delete(req);
-		return luaL_error(L, "fifth argument must be a table");
-	}
-
+	/*
+	 * All user-defined headers must be set before calling
+	 * other httpc module methods so that they take priority
+	 * over the headers that httpc tries to set automatically.
+	 */
 	lua_getfield(L, 5, "headers");
 	if (!lua_isnil(L, -1)) {
 		lua_pushnil(L);
@@ -199,6 +182,32 @@ luaT_httpc_request(lua_State *L)
 	}
 	lua_pop(L, 1);
 
+	const char *method = luaL_checkstring(L, 2);
+	const char *url  = luaL_checkstring(L, 3);
+	if (httpc_set_url(req, method, url) != 0) {
+		httpc_request_delete(req);
+		return luaT_error(L);
+	}
+
+	double timeout = TIMEOUT_INFINITY;
+	int max_header_name_length = HEADER_NAME_LEN;
+	if (lua_isstring(L, 4)) {
+		size_t len = 0;
+		const char *body = lua_tolstring(L, 4, &len);
+		if (len > 0 && httpc_set_body(req, body, len) != 0) {
+			httpc_request_delete(req);
+			return luaT_error(L);
+		}
+	} else if (!lua_isnil(L, 4)) {
+		httpc_request_delete(req);
+		return luaL_error(L, "fourth argument must be a string");
+	}
+
+	if (!lua_istable(L, 5)) {
+		httpc_request_delete(req);
+		return luaL_error(L, "fifth argument must be a table");
+	}
+
 	lua_getfield(L, 5, "ca_path");
 	if (!lua_isnil(L, -1))
 		httpc_set_ca_path(req, lua_tostring(L, -1));
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 8a98d1e92..97326bf12 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -79,6 +79,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)
@@ -471,9 +483,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

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

[Vladimir Davydov]

On Thu, Mar 07, 2019 at 05:48:20PM +0300, Kirill Shcherbatov wrote:
> > I don't like how httpc internal API now looks like: one should set
> > headers before the method/url and keepalive options, otherwise he/she
> > risks loosing them. And there isn't a word about this idiosyncrasy
> > anywhere in the comments.
> > 
> > Also, splitting request construction into allocation (_new) and
> > method/url initialization (_set_url) looks ugly to me, because no
> > request makes sense without them.
> > 
> > May be, we could set default headers in httpc_execute instead?
> No, when those headers are set, they use specific context: like size, idle etx.

You can store them in httpc_request, can't you?

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

[Kirill Shcherbatov]

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

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

[Vladimir Davydov]

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)

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

[Kirill Shcherbatov]

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

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

[Vladimir Davydov]

On Mon, Mar 11, 2019 at 06:57:54PM +0300, Kirill Shcherbatov wrote:
> 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(-)

Pushed to 2.1 and 1.10.