From 62a7e49820b1d962b9895f6ff116e12e95793903 Mon Sep 17 00:00:00 2001 From: Giulio Eulisse <10544+ktf@users.noreply.github.com> Date: Tue, 23 Apr 2024 10:43:56 +0200 Subject: [PATCH] CCDBApi: avoid leaking curl handle. --- CCDB/include/CCDB/CCDBDownloader.h | 4 +- CCDB/src/CCDBDownloader.cxx | 6 ++- CCDB/src/CcdbApi.cxx | 1 + CCDB/test/testCcdbApiDownloader.cxx | 74 +++++++++++------------------ 4 files changed, 38 insertions(+), 47 deletions(-) diff --git a/CCDB/include/CCDB/CCDBDownloader.h b/CCDB/include/CCDB/CCDBDownloader.h index 17b65deb06dc2..e53421dcc26fc 100644 --- a/CCDB/include/CCDB/CCDBDownloader.h +++ b/CCDB/include/CCDB/CCDBDownloader.h @@ -52,6 +52,7 @@ typedef struct DownloaderRequestData { HeaderObjectPair_t hoPair; std::map* headers; std::string userAgent; + curl_slist* optionsList; std::function localContentCallback; } DownloaderRequestData; @@ -296,6 +297,7 @@ class CCDBDownloader int hostInd; int locInd; DownloaderRequestData* requestData; + curl_slist** options; } PerformData; #endif @@ -421,6 +423,6 @@ typedef struct DataForClosingSocket { curl_socket_t socket; } DataForClosingSocket; -} // namespace o2 +} // namespace o2::ccdb #endif // O2_CCDB_CCDBDOWNLOADER_H diff --git a/CCDB/src/CCDBDownloader.cxx b/CCDB/src/CCDBDownloader.cxx index bd2bf22d0add9..8d13368688cb7 100644 --- a/CCDB/src/CCDBDownloader.cxx +++ b/CCDB/src/CCDBDownloader.cxx @@ -535,8 +535,10 @@ void CCDBDownloader::transferFinished(CURL* easy_handle, CURLcode curlCode) } } --(*performData->requestsLeft); + curl_slist_free_all(*performData->options); delete requestData; delete performData->codeDestination; + curl_easy_cleanup(easy_handle); } } break; } @@ -681,6 +683,7 @@ void CCDBDownloader::asynchSchedule(CURL* handle, size_t* requestCounter) curl_easy_getinfo(handle, CURLINFO_PRIVATE, &requestData); headerMap = &(requestData->hoPair.header); hostsPool = &(requestData->hosts); + auto* options = &(requestData->optionsList); // Prepare temporary data about transfer auto* data = new CCDBDownloader::PerformData(); // Freed in transferFinished @@ -692,6 +695,7 @@ void CCDBDownloader::asynchSchedule(CURL* handle, size_t* requestCounter) data->hostInd = 0; data->locInd = 0; data->requestData = requestData; + data->options = options; // Prepare handle and schedule download setHandleOptions(handle, data); @@ -719,4 +723,4 @@ std::string CCDBDownloader::prepareLogMessage(std::string host_url, std::string return fmt::format("CcdbDownloader finished transfer {}{}{} for {} (agent_id: {}) with http code: {}", host_url, (host_url.back() == '/') ? "" : "/", upath, (ts < 0) ? getCurrentTimestamp() : ts, userAgent, httpCode); } -} // namespace o2 +} // namespace o2::ccdb diff --git a/CCDB/src/CcdbApi.cxx b/CCDB/src/CcdbApi.cxx index 46d95bae477d4..4c4aa6ca74a80 100644 --- a/CCDB/src/CcdbApi.cxx +++ b/CCDB/src/CcdbApi.cxx @@ -1656,6 +1656,7 @@ void CcdbApi::scheduleDownload(RequestContext& requestContext, size_t* requestCo data->timestamp = requestContext.timestamp; data->localContentCallback = localContentCallback; data->userAgent = mUniqueAgentID; + data->optionsList = options_list; curl_easy_setopt(curl_handle, CURLOPT_URL, fullUrl.c_str()); initCurlOptionsForRetrieve(curl_handle, (void*)(&data->hoPair), writeCallback, false); diff --git a/CCDB/test/testCcdbApiDownloader.cxx b/CCDB/test/testCcdbApiDownloader.cxx index 68c8333f46ffa..264eff8fdb848 100644 --- a/CCDB/test/testCcdbApiDownloader.cxx +++ b/CCDB/test/testCcdbApiDownloader.cxx @@ -23,7 +23,6 @@ #include #include -#include #include #include @@ -129,7 +128,7 @@ std::vector prepareAsyncHandles(size_t num, std::vectorhoPair.object = dest; - data->hosts.push_back("http://ccdb-test.cern.ch:8080"); + data->hosts.emplace_back("http://ccdb-test.cern.ch:8080"); data->path = "Analysis/ALICE3/Centrality"; data->timestamp = 1646729604010; data->localContentCallback = nullptr; @@ -164,15 +163,20 @@ BOOST_AUTO_TEST_CASE(asynch_schedule_test) } while (transfersLeft > 0) { - downloader.runLoop(0); + downloader.runLoop(false); } for (int i = 0; i < TRANSFERS; i++) { - long httpCode; - curl_easy_getinfo(handles[i], CURLINFO_HTTP_CODE, &httpCode); - BOOST_CHECK(httpCode == 200); - BOOST_CHECK(dests[i]->size() != 0); - curl_easy_cleanup(handles[i]); + // I would claim that accessing the handles after they are complete + // is actually not supported by the current API, because it was + // previously relying on leaking the handles. Disabling the whole + // thing until we verify that's actually the case. + // + // long httpCode; + // curl_easy_getinfo(handles[i], CURLINFO_HTTP_CODE, &httpCode); + // BOOST_CHECK_EQUAL(httpCode, 200); + // BOOST_CHECK_NE(dests[i]->size(), 0); + // curl_easy_cleanup(handles[i]); delete dests[i]; } curl_global_cleanup(); @@ -191,13 +195,11 @@ BOOST_AUTO_TEST_CASE(perform_test) CURLcode curlCode = downloader.perform(handle); - BOOST_CHECK(curlCode == CURLE_OK); - std::cout << "CURL code: " << curlCode << "\n"; + BOOST_CHECK_EQUAL(curlCode, CURLE_OK); long httpCode; curl_easy_getinfo(handle, CURLINFO_HTTP_CODE, &httpCode); - BOOST_CHECK(httpCode == 200); - std::cout << "HTTP code: " << httpCode << "\n"; + BOOST_CHECK_EQUAL(httpCode, 200); curl_easy_cleanup(handle); curl_global_cleanup(); @@ -220,19 +222,13 @@ BOOST_AUTO_TEST_CASE(blocking_batch_test) auto curlCodes = downloader.batchBlockingPerform(handleVector); for (CURLcode code : curlCodes) { - BOOST_CHECK(code == CURLE_OK); - if (code != CURLE_OK) { - std::cout << "CURL Code: " << code << "\n"; - } + BOOST_CHECK_EQUAL(code, CURLE_OK); } for (CURL* handle : handleVector) { long httpCode; curl_easy_getinfo(handle, CURLINFO_HTTP_CODE, &httpCode); - BOOST_CHECK(httpCode == 200); - if (httpCode != 200) { - std::cout << "HTTP Code: " << httpCode << "\n"; - } + BOOST_CHECK_EQUAL(httpCode, 200); curl_easy_cleanup(handle); } @@ -261,19 +257,13 @@ BOOST_AUTO_TEST_CASE(test_with_break) auto curlCodes = downloader.batchBlockingPerform(handleVector); for (CURLcode code : curlCodes) { - BOOST_CHECK(code == CURLE_OK); - if (code != CURLE_OK) { - std::cout << "CURL Code: " << code << "\n"; - } + BOOST_CHECK_EQUAL(code, CURLE_OK); } for (CURL* handle : handleVector) { long httpCode; curl_easy_getinfo(handle, CURLINFO_HTTP_CODE, &httpCode); - BOOST_CHECK(httpCode == 200); - if (httpCode != 200) { - std::cout << "HTTP Code: " << httpCode << "\n"; - } + BOOST_CHECK_EQUAL(httpCode, 200); curl_easy_cleanup(handle); } @@ -292,19 +282,13 @@ BOOST_AUTO_TEST_CASE(test_with_break) auto curlCodes2 = downloader.batchBlockingPerform(handleVector2); for (CURLcode code : curlCodes2) { - BOOST_CHECK(code == CURLE_OK); - if (code != CURLE_OK) { - std::cout << "CURL Code: " << code << "\n"; - } + BOOST_CHECK_EQUAL(code, CURLE_OK); } for (CURL* handle : handleVector2) { long httpCode; curl_easy_getinfo(handle, CURLINFO_HTTP_CODE, &httpCode); - BOOST_CHECK(httpCode == 200); - if (httpCode != 200) { - std::cout << "HTTP Code: " << httpCode << "\n"; - } + BOOST_CHECK_EQUAL(httpCode, 200); curl_easy_cleanup(handle); } @@ -357,18 +341,18 @@ BOOST_AUTO_TEST_CASE(external_loop_test) CURLcode curlCode = downloader->perform(handle); - BOOST_CHECK(curlCode == CURLE_OK); + BOOST_CHECK_EQUAL(curlCode, CURLE_OK); long httpCode; curl_easy_getinfo(handle, CURLINFO_HTTP_CODE, &httpCode); - BOOST_CHECK(httpCode == 200); + BOOST_CHECK_EQUAL(httpCode, 200); curl_easy_cleanup(handle); curl_global_cleanup(); // Check if test timer and external loop are still alive - BOOST_CHECK(uv_is_active((uv_handle_t*)testTimer) != 0); - BOOST_CHECK(uv_loop_alive(uvLoop) != 0); + BOOST_CHECK_NE(uv_is_active((uv_handle_t*)testTimer), 0); + BOOST_CHECK_NE(uv_loop_alive(uvLoop), 0); // Downloader must be closed before uv_loop. // The reason for that are the uv_poll handles attached to the curl multi handle. @@ -384,11 +368,11 @@ BOOST_AUTO_TEST_CASE(external_loop_test) BOOST_AUTO_TEST_CASE(trim_host_url_test) { CCDBDownloader downloader; - BOOST_CHECK(downloader.trimHostUrl("http://localhost:8080") == "http://localhost:8080"); - BOOST_CHECK(downloader.trimHostUrl("http://localhost") == "http://localhost"); - BOOST_CHECK(downloader.trimHostUrl("http://localhost:8080/some/path") == "http://localhost:8080"); - BOOST_CHECK(downloader.trimHostUrl("http://localhost/some/path") == "http://localhost"); - BOOST_CHECK(downloader.trimHostUrl("http://localhost:8080/Task/Detector/1?HTTPOnly=true") == "http://localhost:8080"); + BOOST_CHECK_EQUAL(downloader.trimHostUrl("http://localhost:8080"), "http://localhost:8080"); + BOOST_CHECK_EQUAL(downloader.trimHostUrl("http://localhost"), "http://localhost"); + BOOST_CHECK_EQUAL(downloader.trimHostUrl("http://localhost:8080/some/path"), "http://localhost:8080"); + BOOST_CHECK_EQUAL(downloader.trimHostUrl("http://localhost/some/path"), "http://localhost"); + BOOST_CHECK_EQUAL(downloader.trimHostUrl("http://localhost:8080/Task/Detector/1?HTTPOnly=true"), "http://localhost:8080"); } } // namespace ccdb