Skip to content
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

Jump to bottom

prevent leaking of CURL handles #36

Merged
merged 1 commit into from
May 31, 2023
Merged

prevent leaking of CURL handles #36

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
1 change: 1 addition & 0 deletions src/datadog/curl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@ void CurlImpl::run() {
}

log_on_error(curl_.multi_remove_handle(multi_handle_, handle));
curl_.easy_cleanup(handle);
}

request_handles_.clear();
Expand Down
255 changes: 172 additions & 83 deletions test/test_curl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,103 +9,131 @@
#include <chrono>
#include <exception>
#include <system_error>
#include <unordered_set>

#include "mocks/loggers.h"
#include "test.h"

using namespace datadog::tracing;

TEST_CASE("parse response headers and body") {
class MockCurlLibrary : public CurlLibrary {
void *user_data_on_header_ = nullptr;
HeaderCallback on_header_ = nullptr;
void *user_data_on_write_ = nullptr;
WriteCallback on_write_ = nullptr;
CURL *handle_ = nullptr;
CURLMsg message_;

CURLcode easy_getinfo_response_code(CURL *, long *code) override {
*code = 200;
return CURLE_OK;
}
CURLcode easy_setopt_headerdata(CURL *, void *data) override {
user_data_on_header_ = data;
return CURLE_OK;
}
CURLcode easy_setopt_headerfunction(CURL *,
HeaderCallback on_header) override {
on_header_ = on_header;
return CURLE_OK;
}
CURLcode easy_setopt_writedata(CURL *, void *data) override {
user_data_on_write_ = data;
return CURLE_OK;
}
class SingleRequestMockCurlLibrary : public CurlLibrary {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is moved and renamed from the class MockCurlLibrary that used to be in the TEST_CASE("parse response headers and body"). It's used there and in the new test case.

I also added data members created_handles_, destroyed_handles_, message_result_, and delay_message_.

public:
void *user_data_on_header_ = nullptr;
HeaderCallback on_header_ = nullptr;
void *user_data_on_write_ = nullptr;
WriteCallback on_write_ = nullptr;
CURL *added_handle_ = nullptr;
CURLMsg message_;
// Since `SingleRequestMockCurlLibrary` supports at most one request,
// `created_handles_` and `destroyed_handles_` will have size zero or one.
std::unordered_multiset<CURL *> created_handles_;
std::unordered_multiset<CURL *> destroyed_handles_;
// `message_result_` is the success/error code associated with the "done"
// message sent to the event loop when the request has finished.
CURLcode message_result_ = CURLE_OK;
// `delay_message_` is used to prevent the immediate dispatch of a "done"
// message to the event loop. This allows races to be explored between request
// registration and `Curl` shutdown.
bool delay_message_ = false;

void easy_cleanup(CURL *handle) override {
destroyed_handles_.insert(handle);
CurlLibrary::easy_cleanup(handle);
}

CURLcode easy_setopt_writefunction(CURL *,
WriteCallback on_write) override {
on_write_ = on_write;
return CURLE_OK;
}
CURLMcode multi_add_handle(CURLM *, CURL *easy_handle) override {
handle_ = easy_handle;
return CURLM_OK;
}
CURLMsg *multi_info_read(CURLM *, int *msgs_in_queue) override {
*msgs_in_queue = handle_ != nullptr;
if (*msgs_in_queue == 0) {
return nullptr;
}
message_.msg = CURLMSG_DONE;
message_.easy_handle = handle_;
message_.data.result = CURLE_OK;
return &message_;
}
CURLMcode multi_perform(CURLM *, int *running_handles) override {
if (!handle_) {
*running_handles = 0;
return CURLM_OK;
}
CURL *easy_init() override {
CURL *handle = CurlLibrary::easy_init();
created_handles_.insert(handle);
return handle;
}

// If any of these `REQUIRE`s fail, an exception will be thrown and the
// test will abort. The runtime will print the exception first, though.
REQUIRE(on_header_);
REQUIRE(user_data_on_header_);
*running_handles = 1;
std::string header = "200 OK";
REQUIRE(on_header_(header.data(), 1, header.size(),
user_data_on_header_) == header.size());
header = "Foo-Bar: baz";
REQUIRE(on_header_(header.data(), 1, header.size(),
user_data_on_header_) == header.size());
header = "BOOM-BOOM: boom, boom, boom, boom ";
REQUIRE(on_header_(header.data(), 1, header.size(),
user_data_on_header_) == header.size());
header = "BOOM-boom: ignored";
REQUIRE(on_header_(header.data(), 1, header.size(),
user_data_on_header_) == header.size());

REQUIRE(on_write_);
REQUIRE(user_data_on_write_);
std::string body = "{\"message\": \"Dogs don't know it's not libcurl!\"}";
// Send the body in two pieces.
REQUIRE(on_write_(body.data(), 1, body.size() / 2, user_data_on_write_) ==
body.size() / 2);
const auto remaining = body.size() - (body.size() / 2);
REQUIRE(on_write_(body.data() + body.size() / 2, 1, remaining,
user_data_on_write_) == remaining);
CURLcode easy_getinfo_response_code(CURL *, long *code) override {
*code = 200;
return CURLE_OK;
}
CURLcode easy_setopt_headerdata(CURL *, void *data) override {
user_data_on_header_ = data;
return CURLE_OK;
}
CURLcode easy_setopt_headerfunction(CURL *,
HeaderCallback on_header) override {
on_header_ = on_header;
return CURLE_OK;
}
CURLcode easy_setopt_writedata(CURL *, void *data) override {
user_data_on_write_ = data;
return CURLE_OK;
}

return CURLM_OK;
CURLcode easy_setopt_writefunction(CURL *, WriteCallback on_write) override {
on_write_ = on_write;
return CURLE_OK;
}
CURLMcode multi_add_handle(CURLM *, CURL *easy_handle) override {
added_handle_ = easy_handle;
return CURLM_OK;
}
CURLMsg *multi_info_read(CURLM *, int *msgs_in_queue) override {
if (delay_message_) {
*msgs_in_queue = 0;
return nullptr;
}
CURLMcode multi_remove_handle(CURLM *, CURL *easy_handle) override {
REQUIRE(easy_handle == handle_);
handle_ = nullptr;

*msgs_in_queue = added_handle_ != nullptr;
if (*msgs_in_queue == 0) {
return nullptr;
}
message_.msg = CURLMSG_DONE;
message_.easy_handle = added_handle_;
message_.data.result = message_result_;
return &message_;
}
CURLMcode multi_perform(CURLM *, int *running_handles) override {
if (!added_handle_) {
*running_handles = 0;
return CURLM_OK;
}
};

// If any of these `REQUIRE`s fail, an exception will be thrown and the
// test will abort. The runtime will print the exception first, though.
REQUIRE(on_header_);
REQUIRE(user_data_on_header_);
*running_handles = 1;
std::string header = "200 OK";
REQUIRE(on_header_(header.data(), 1, header.size(), user_data_on_header_) ==
header.size());
header = "Foo-Bar: baz";
REQUIRE(on_header_(header.data(), 1, header.size(), user_data_on_header_) ==
header.size());
header = "BOOM-BOOM: boom, boom, boom, boom ";
REQUIRE(on_header_(header.data(), 1, header.size(), user_data_on_header_) ==
header.size());
header = "BOOM-boom: ignored";
REQUIRE(on_header_(header.data(), 1, header.size(), user_data_on_header_) ==
header.size());

REQUIRE(on_write_);
REQUIRE(user_data_on_write_);
std::string body = "{\"message\": \"Dogs don't know it's not libcurl!\"}";
// Send the body in two pieces.
REQUIRE(on_write_(body.data(), 1, body.size() / 2, user_data_on_write_) ==
body.size() / 2);
const auto remaining = body.size() - (body.size() / 2);
REQUIRE(on_write_(body.data() + body.size() / 2, 1, remaining,
user_data_on_write_) == remaining);

return CURLM_OK;
}
CURLMcode multi_remove_handle(CURLM *, CURL *easy_handle) override {
REQUIRE(easy_handle == added_handle_);
added_handle_ = nullptr;
return CURLM_OK;
}
};

TEST_CASE("parse response headers and body") {
const auto logger = std::make_shared<MockLogger>();
MockCurlLibrary library;
SingleRequestMockCurlLibrary library;
const auto client = std::make_shared<Curl>(logger, library);

SECTION("in the tracer") {
Expand Down Expand Up @@ -207,6 +235,7 @@ TEST_CASE("fail to allocate request handle") {
// Each call to `Curl::post` allocates a new "easy handle." If that fails,
// then `post` immediately returns an error.
class MockCurlLibrary : public CurlLibrary {
public:
CURL *easy_init() override { return nullptr; }
};

Expand Down Expand Up @@ -344,5 +373,65 @@ TEST_CASE("setopt failures") {
REQUIRE(result.error().code == Error::CURL_REQUEST_SETUP_FAILED);
}

TEST_CASE("handles are always cleaned up") {
const auto logger = std::make_shared<MockLogger>();
SingleRequestMockCurlLibrary library;
auto client = std::make_shared<Curl>(logger, library);

SECTION("when the response is delivered") {
Optional<Error> post_error;
std::exception_ptr exception;
const HTTPClient::URL url = {"http", "whatever", ""};
const auto result = client->post(
url, [](const auto &) {}, "whatever",
[&](int status, const DictReader & /*headers*/, std::string body) {
try {
REQUIRE(status == 200);
REQUIRE(body ==
"{\"message\": \"Dogs don't know it's not libcurl!\"}");
} catch (...) {
exception = std::current_exception();
}
},
[&](const Error &error) { post_error = error; });

REQUIRE(result);
client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1));
if (exception) {
std::rethrow_exception(exception);
}
REQUIRE_FALSE(post_error);
}

SECTION("when an error occurs") {
Optional<Error> post_error;
const HTTPClient::URL url = {"http", "whatever", ""};
const auto ignore = [](auto &&...) {};
library.message_result_ = CURLE_COULDNT_CONNECT; // any error would do
const auto result =
client->post(url, ignore, "whatever", ignore,
[&](const Error &error) { post_error = error; });

REQUIRE(result);
client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1));
REQUIRE(post_error);
}

SECTION("when we shut down while a request is in flight") {
const HTTPClient::URL url = {"http", "whatever", ""};
const auto ignore = [](auto &&...) {};
library.delay_message_ = true;
const auto result = client->post(url, ignore, "whatever", ignore, ignore);

REQUIRE(result);
// Destroy the `Curl` object.
client.reset();
}

// Here are the checks relevant to this test.
REQUIRE(library.created_handles_.size() == 1);
REQUIRE(library.created_handles_ == library.destroyed_handles_);
Comment on lines +431 to +433
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SECTIONs above are all setup. The post-condition, here, is always the same: one handle created and one handle destroyed.

}

// TODO: "multi_*" failures
// TODO: "getinfo" failures
Loading