[tarantool-patches] [PATCH] http.client: Fix waiting after received result

[Ilya Markov <imarkov@tarantool.org>]

Current implementation of http.client relies on fiber_cond which is set
after the request was registered and doesn't consider the fact that
response may be handled before the set of fiber_cond.

So we may have the following situation:
1. Register request in libcurl(curl_multi_add_handle in curl_execute).
2. Receive and process response, fiber_cond_signal on cond_var which no
one waits.
3. fiber_cond_wait on cond which is already signaled. Wait until timeout
is fired.

In this case user have to wait timeout, though data was received
earlier.

Fix this with adding extra flag in_progress to curl_request struct.
Set this flag true before registering request in libcurl and set it
false when request is finished before fiber_cond_signal.
When in_progress flag is false, don't wait on cond variable.

Add 1 error injection.

Closes #3452
---
The patch is based on 1.9

https://github.com/tarantool/tarantool/issues/3452
https://github.com/tarantool/tarantool/tree/gh-3452-timeout-http-client

 src/curl.c                        | 22 ++++++++++++++++++----
 src/curl.h                        |  2 ++
 src/errinj.h                      |  1 +
 test/app-tap/http_client.test.lua | 21 +++++++++++++++------
 4 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/src/curl.c b/src/curl.c
index df95713..b85a455 100644
--- a/src/curl.c
+++ b/src/curl.c
@@ -35,6 +35,7 @@
 #include <curl/curl.h>

 #include "fiber.h"
+#include "errinj.h"

 /**
  * Process events
@@ -82,6 +83,13 @@ curl_multi_process(CURLM *multi, curl_socket_t sockfd, int events)
 		struct curl_request *request = NULL;
 		curl_easy_getinfo(easy, CURLINFO_PRIVATE, (void *) &request);
 		request->code = (int) code;
+		request->in_progress = false;
+#ifndef NDEBUG
+		struct errinj *errinj = errinj(ERRINJ_HTTP_RESPONSE_ADD_WAIT,
+					       ERRINJ_BOOL);
+		if (errinj != NULL)
+			errinj->bparam = false;
+#endif
 		fiber_cond_signal(&request->cond);
 	}
 }
@@ -270,6 +278,7 @@ curl_request_create(struct curl_request *curl_request)
 		diag_set(OutOfMemory, 0, "curl", "easy");
 		return -1;
 	}
+	curl_request->in_progress = false;
 	curl_request->code = CURLE_OK;
 	fiber_cond_create(&curl_request->cond);
 	return 0;
@@ -288,13 +297,18 @@ curl_execute(struct curl_request *curl_request, struct curl_env *env,
 	     double timeout)
 {
 	CURLMcode mcode;
-
+	curl_request->in_progress = true;
 	mcode = curl_multi_add_handle(env->multi, curl_request->easy);
 	if (mcode != CURLM_OK)
 		goto curl_merror;
-
-	/* Don't wait on a cond if request has already failed */
-	if (curl_request->code == CURLE_OK) {
+#ifndef NDEBUG
+	struct errinj *errinj = errinj(ERRINJ_HTTP_RESPONSE_ADD_WAIT,
+				       ERRINJ_BOOL);
+	while (errinj != NULL && errinj->bparam)
+		fiber_sleep(0.001);
+#endif
+	/* Don't wait on a cond if request has already failed or finished. */
+	if (curl_request->code == CURLE_OK && curl_request->in_progress) {
 		++env->stat.active_requests;
 		int rc = fiber_cond_wait_timeout(&curl_request->cond, timeout);
 		if (rc < 0 || fiber_is_cancelled())
diff --git a/src/curl.h b/src/curl.h
index 21ba475..cf91636 100644
--- a/src/curl.h
+++ b/src/curl.h
@@ -68,6 +68,8 @@ struct curl_env {
 struct curl_request {
 	/** Internal libcurl status code. */
 	int code;
+	/** States that request is running. */
+	bool in_progress;
 	/** Information associated with a specific easy handle. */
 	CURL *easy;
 	/**
diff --git a/src/errinj.h b/src/errinj.h
index 4998fdc..5694d3f 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -113,6 +113,7 @@ struct errinj {
 	_(ERRINJ_LOG_ROTATE, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_SNAP_COMMIT_DELAY, ERRINJ_BOOL, {.bparam = 0}) \
 	_(ERRINJ_SNAP_WRITE_ROW_TIMEOUT, ERRINJ_DOUBLE, {.dparam = 0}) \
+	_(ERRINJ_HTTP_RESPONSE_ADD_WAIT, ERRINJ_BOOL, {.bparam = false}) \

 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 887395d..37ba1ae 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -79,19 +79,28 @@ local function test_http_client(test, url, opts)
     test:is(r.status, 200, 'request')
 end

-local function test_cancel_and_timeout(test, url, opts)
-    test:plan(2)
+local function test_cancel_and_errinj(test, url, opts)
+    test:plan(3)
     local ch = fiber.channel(1)
     local http = client:new()
-    local f = fiber.create(function()
-                ch:put(http:get(url, opts)) end)
+    local func  = function(fopts)
+        ch:put(http:get(url, fopts))
+    end
+    local f = fiber.create(func, opts)
     f:cancel()
     local r = ch:get()
     test:ok(r.status == 408 and string.find(r.reason, "Timeout"),
                     "After cancel fiber timeout is returned")
-    local r = http:get(url, merge(opts, {timeout = 0.0001}))
+    r = http:get(url, merge(opts, {timeout = 0.0001}))
     test:ok(r.status == 408 and string.find(r.reason, "Timeout"),
                                                        "Timeout check")
+    local errinj = box.error.injection
+    errinj.set('ERRINJ_HTTP_RESPONSE_ADD_WAIT', true)
+    local topts = merge(opts, {timeout = 1200})
+    f = fiber.create(func, topts)
+    r = ch:get()
+    test:is(r.status, 200, "No hangs in errinj")
+    errinj.set('ERRINJ_HTTP_RESPONSE_ADD_WAIT', false)
 end

 local function test_post_and_get(test, url, opts)
@@ -384,7 +393,7 @@ function run_tests(test, sock_family, sock_addr)
     test:plan(9)
     local server, url, opts = start_server(test, sock_family, sock_addr)
     test:test("http.client", test_http_client, url, opts)
-    test:test("cancel and timeout", test_cancel_and_timeout, url, opts)
+    test:test("cancel and errinj", test_cancel_and_errinj, url, opts)
     test:test("basic http post/get", test_post_and_get, url, opts)
     test:test("errors", test_errors)
     test:test("headers", test_headers, url, opts)
--
2.7.4