From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4FB20203DD for ; Tue, 19 Jun 2018 11:08:14 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Fg4wlW3m1ruo for ; Tue, 19 Jun 2018 11:08:14 -0400 (EDT) Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id F40AD20C38 for ; Tue, 19 Jun 2018 11:08:13 -0400 (EDT) From: Ilya Markov Subject: [tarantool-patches] [PATCH] http.client: Fix waiting after received result Date: Tue, 19 Jun 2018 18:08:02 +0300 Message-Id: <1529420882-24447-1-git-send-email-imarkov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: georgy@tarantool.org Cc: tarantool-patches@freelists.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 #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