From 910777efbc2dfc777aba990539c5a09489eef609 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Tue, 18 Dec 2018 14:47:08 +0200 Subject: [PATCH] MXS-2208 Address review comments Also report errors in more detailed manner. --- maxutils/maxbase/include/maxbase/http.hh | 25 ++++++++ maxutils/maxbase/src/http.cc | 74 ++++++++++++++++-------- maxutils/maxbase/src/test/test_http.cc | 3 +- 3 files changed, 77 insertions(+), 25 deletions(-) diff --git a/maxutils/maxbase/include/maxbase/http.hh b/maxutils/maxbase/include/maxbase/http.hh index f8f031869..4136edc71 100644 --- a/maxutils/maxbase/include/maxbase/http.hh +++ b/maxutils/maxbase/include/maxbase/http.hh @@ -69,6 +69,13 @@ struct Config struct Result { + enum + { + ERROR = -1, // Some non-specific error occurred. + COULDNT_RESOLVE_HOST = -2, // The specified host cold not be resolved. + OPERATION_TIMEDOUT = -3 // The operation timed out. + }; + int code = 0; // HTTP response code std::string body; // Response body std::map headers; // Headers attached to the response @@ -80,6 +87,9 @@ struct Result * @param url The URL to GET. * @param config The config to use. * + * @note The @c url is assumed to be escaped in case it contain arguments + * that must be escaped. + * * @return A @c Result. */ Result get(const std::string& url, const Config& config = Config()); @@ -92,6 +102,9 @@ Result get(const std::string& url, const Config& config = Config()); * @param password Password for the user. * @param config The config to use. * + * @note The @c url is assumed to be escaped in case it contain arguments + * that must be escaped, but @c user and @c pass will always be escaped. + * * @return A @c Result. */ Result get(const std::string& url, @@ -104,6 +117,9 @@ Result get(const std::string& url, * @param urls The URLs to GET. * @param config The config to use. * + * @note The @c urls are assumed to be escaped in case they contain arguments + * that must be escaped. + * * @return A @c Result. */ std::vector get(const std::vector& urls, @@ -117,6 +133,9 @@ std::vector get(const std::vector& urls, * @param password Password for the user. * @param config The config to use. * + * @note The @c urls are assumed to be escaped in case they contain arguments + * that must be escaped, but @c user and @c pass will always be escaped. + * * @return Vector of @c Results, as many as there were @c urls. */ std::vector get(const std::vector& urls, @@ -259,6 +278,9 @@ const char* to_string(Async::status_t status); * @param urls The URLs to GET. * @param config The config to use. * + * @note The @c urls are assumed to be escaped in case they contain arguments + * that must be escaped. + * * @return An Async instance using which the operation can be performed. */ Async get_async(const std::vector& urls, @@ -272,6 +294,9 @@ Async get_async(const std::vector& urls, * @param password Password for the user. * @param config The config to use. * + * @note The @c urls are assumed to be escaped in case they contains arguments + * that must be escaped, but @c user and @c pass will always be escaped. + * * @return An Async instance using which the operation can be performed. */ Async get_async(const std::vector& urls, diff --git a/maxutils/maxbase/src/http.cc b/maxutils/maxbase/src/http.cc index b27dfdfa4..9ca6d0496 100644 --- a/maxutils/maxbase/src/http.cc +++ b/maxutils/maxbase/src/http.cc @@ -44,6 +44,24 @@ static struct THIS_UNIT using namespace mxb; using namespace mxb::http; +int translate_curl_code(CURLcode code) +{ + switch (code) + { + case CURLE_OK: + return 0; + + case CURLE_COULDNT_RESOLVE_HOST: + return Result::COULDNT_RESOLVE_HOST; + + case CURLE_OPERATION_TIMEDOUT: + return Result::OPERATION_TIMEDOUT; + + default: + return Result::ERROR; + } +} + template inline int checked_curl_setopt(CURL* pCurl, CURLoption option, T value) { @@ -108,8 +126,8 @@ CURL* get_easy_curl(const std::string& url, if (pCurl) { checked_curl_setopt(pCurl, CURLOPT_NOSIGNAL, 1); - checked_curl_setopt(pCurl, CURLOPT_CONNECTTIMEOUT, config.connect_timeout_s); // For connection phase - checked_curl_setopt(pCurl, CURLOPT_TIMEOUT, config.timeout_s); // For data transfer phase + checked_curl_setopt(pCurl, CURLOPT_CONNECTTIMEOUT, config.connect_timeout_s);// For connection phase + checked_curl_setopt(pCurl, CURLOPT_TIMEOUT, config.timeout_s); // For data transfer phase checked_curl_setopt(pCurl, CURLOPT_ERRORBUFFER, pErrbuf); checked_curl_setopt(pCurl, CURLOPT_WRITEFUNCTION, write_callback); checked_curl_setopt(pCurl, CURLOPT_WRITEDATA, &pRes->body); @@ -296,10 +314,8 @@ public: mxb_assert(!true); break; - default: + case Async::PENDING: { - mxb_assert(m_status == Async::PENDING); - fd_set fdread; fd_set fdwrite; fd_set fdexcep; @@ -367,12 +383,15 @@ public: Result* pResult = context.pResult; Errbuf* pErrbuf = context.pErrbuf; - long code; - curl_easy_getinfo(pCurl, CURLINFO_RESPONSE_CODE, &code); - pResult->code = code; - - if ((code == 0) && pResult->body.empty()) + if (pMsg->data.result == CURLE_OK) { + long code; + curl_easy_getinfo(pCurl, CURLINFO_RESPONSE_CODE, &code); + pResult->code = code; + } + else + { + pResult->code = translate_curl_code(pMsg->data.result); pResult->body = pErrbuf->data(); } @@ -506,15 +525,20 @@ Result get(const std::string& url, const std::string& user, const std::string& p CURL* pCurl = get_easy_curl(url, user, password, config, &res, errbuf); mxb_assert(pCurl); - if (curl_easy_perform(pCurl) == CURLE_OK) + CURLcode code = curl_easy_perform(pCurl); + + switch (code) { - long code = 0; // needs to be a long - curl_easy_getinfo(pCurl, CURLINFO_RESPONSE_CODE, &code); - res.code = code; - } - else - { - res.code = -1; + case CURLE_OK: + { + long code = 0; // needs to be a long + curl_easy_getinfo(pCurl, CURLINFO_RESPONSE_CODE, &code); + res.code = code; + } + break; + + default: + res.code = translate_curl_code(code); res.body = errbuf; } @@ -534,16 +558,18 @@ vector get(const std::vector& urls, { Async http = get_async(urls, user, password, config); - while (http.perform() == Async::PENDING) + long timeout_ms = (config.connect_timeout_s + config.timeout_s) * 1000; + long max_wait_ms = timeout_ms / 10; + + long wait_ms = 10; + while (http.perform(wait_ms) == Async::PENDING) { - long ms = http.wait_no_more_than(); + wait_ms = http.wait_no_more_than(); - if (ms > 100) + if (wait_ms > max_wait_ms) { - ms = 100; + wait_ms = max_wait_ms; } - - std::this_thread::sleep_for(std::chrono::milliseconds(ms)); } vector results(http.results()); diff --git a/maxutils/maxbase/src/test/test_http.cc b/maxutils/maxbase/src/test/test_http.cc index 7795075a3..c9414e1d0 100644 --- a/maxutils/maxbase/src/test/test_http.cc +++ b/maxutils/maxbase/src/test/test_http.cc @@ -60,7 +60,8 @@ int check_results(const vector& urls, auto& res = results[i]; bool expected_success = expected_successes[i]; - cout << url << " responded with: " << res.code << endl; + cout << url << " responded with: " << res.code + << (res.code < 0 ? (", " + res.body) : "") << endl; if (expected_success) {