Skip to content

CCDBApi: avoid leaking curl handle. #13061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
martenole merged 1 commit into from
Apr 23, 2024
Merged

CCDBApi: avoid leaking curl handle. #13061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CCDB/include/CCDB/CCDBDownloader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ typedef struct DownloaderRequestData {
HeaderObjectPair_t hoPair;
std::map<std::string, std::string>* headers;
std::string userAgent;
curl_slist* optionsList;

std::function<bool(std::string)> localContentCallback;
} DownloaderRequestData;
Expand Down Expand Up @@ -296,6 +297,7 @@ class CCDBDownloader
int hostInd;
int locInd;
DownloaderRequestData* requestData;
curl_slist** options;
} PerformData;
#endif

Expand Down Expand Up @@ -421,6 +423,6 @@ typedef struct DataForClosingSocket {
curl_socket_t socket;
} DataForClosingSocket;

} // namespace o2
} // namespace o2::ccdb

#endif // O2_CCDB_CCDBDOWNLOADER_H
6 changes: 5 additions & 1 deletion CCDB/src/CCDBDownloader.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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
1 change: 1 addition & 0 deletions CCDB/src/CcdbApi.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
74 changes: 29 additions & 45 deletions CCDB/test/testCcdbApiDownloader.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
#include <fairlogger/Logger.h>

#include <boost/test/unit_test.hpp>
#include <boost/optional/optional.hpp>
#include <boost/asio/ip/host_name.hpp>
#include <uv.h>

Expand Down Expand Up @@ -129,7 +128,7 @@ std::vector<CURL*> prepareAsyncHandles(size_t num, std::vector<o2::pmr::vector<c

auto data = new DownloaderRequestData();
data->hoPair.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;
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
}

Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down