Unverified Commit b83ebff8 authored by Noir's avatar Noir Committed by GitHub

Merge pull request #1322

Fix memory leak in the message builder
parents d55bfb0c 34a67368
...@@ -29,6 +29,8 @@ is based on [Keep a Changelog](https://keepachangelog.com). ...@@ -29,6 +29,8 @@ is based on [Keep a Changelog](https://keepachangelog.com).
newer EC-curve API. newer EC-curve API.
- When working with settings, `put`, `put_missing`, `get_if`, etc. now - When working with settings, `put`, `put_missing`, `get_if`, etc. now
gracefully handle the `global` category when explicitly using it. gracefully handle the `global` category when explicitly using it.
- Messages created from a `message_builder` did not call the destructors for
their values, potentially causing memory leaks (#1321).
### Changed ### Changed
......
...@@ -17,6 +17,10 @@ namespace caf::detail { ...@@ -17,6 +17,10 @@ namespace caf::detail {
/// later. /// later.
class CAF_CORE_EXPORT message_builder_element { class CAF_CORE_EXPORT message_builder_element {
public: public:
message_builder_element() = default;
message_builder_element(const message_builder_element&) = delete;
message_builder_element& operator=(const message_builder_element&) = delete;
virtual ~message_builder_element(); virtual ~message_builder_element();
/// Uses placement new to create a copy of the wrapped value at given memory /// Uses placement new to create a copy of the wrapped value at given memory
...@@ -34,10 +38,24 @@ public: ...@@ -34,10 +38,24 @@ public:
template <class T> template <class T>
class message_builder_element_impl : public message_builder_element { class message_builder_element_impl : public message_builder_element {
public: public:
message_builder_element_impl(T value) : value_(std::move(value)) { template <class Value>
explicit message_builder_element_impl(Value&& value)
: value_(std::forward<Value>(value)) {
// nop // nop
} }
message_builder_element_impl() = delete;
message_builder_element_impl(message_builder_element_impl&&) = delete;
message_builder_element_impl(const message_builder_element_impl&) = delete;
message_builder_element_impl& operator=(message_builder_element_impl&&)
= delete;
message_builder_element_impl& operator=(const message_builder_element_impl&)
= delete;
byte* copy_init(byte* storage) const override { byte* copy_init(byte* storage) const override {
new (storage) T(value_); new (storage) T(value_);
return storage + padded_size_v<T>; return storage + padded_size_v<T>;
...@@ -54,10 +72,4 @@ private: ...@@ -54,10 +72,4 @@ private:
using message_builder_element_ptr = std::unique_ptr<message_builder_element>; using message_builder_element_ptr = std::unique_ptr<message_builder_element>;
template <class T>
auto make_message_builder_element(T&& x) {
using impl = message_builder_element_impl<std::decay_t<T>>;
return message_builder_element_ptr{new impl(std::forward<T>(x))};
}
} // namespace caf::detail } // namespace caf::detail
...@@ -45,12 +45,12 @@ public: ...@@ -45,12 +45,12 @@ public:
/// Adds `x` to the elements of the buffer. /// Adds `x` to the elements of the buffer.
template <class T> template <class T>
message_builder& append(T&& x) { message_builder& append(T&& x) {
using namespace detail; using value_type = detail::strip_and_convert_t<T>;
using value_type = strip_and_convert_t<T>; static_assert(detail::sendable<value_type>);
static_assert(sendable<value_type>); using wrapper_type = detail::message_builder_element_impl<value_type>;
storage_size_ += padded_size_v<value_type>; storage_size_ += detail::padded_size_v<value_type>;
types_.push_back(type_id_v<value_type>); types_.push_back(type_id_v<value_type>);
elements_.emplace_back(make_message_builder_element(std::forward<T>(x))); elements_.emplace_back(std::make_unique<wrapper_type>(std::forward<T>(x)));
return *this; return *this;
} }
......
...@@ -33,11 +33,14 @@ message to_message_impl(size_t storage_size, TypeListBuilder& types, ...@@ -33,11 +33,14 @@ message to_message_impl(size_t storage_size, TypeListBuilder& types,
raw_ptr = new (vptr) message_data(types.copy_to_list()); raw_ptr = new (vptr) message_data(types.copy_to_list());
intrusive_cow_ptr<message_data> ptr{raw_ptr, false}; intrusive_cow_ptr<message_data> ptr{raw_ptr, false};
auto storage = raw_ptr->storage(); auto storage = raw_ptr->storage();
for (auto& element : elements) for (auto& element : elements) {
if constexpr (Policy == move_msg) if constexpr (Policy == move_msg) {
storage = element->move_init(storage); storage = element->move_init(storage);
else } else {
storage = element->copy_init(storage); storage = element->copy_init(storage);
}
raw_ptr->inc_constructed_elements();
}
return message{std::move(ptr)}; return message{std::move(ptr)};
} }
......
...@@ -16,38 +16,68 @@ ...@@ -16,38 +16,68 @@
#include "caf/message.hpp" #include "caf/message.hpp"
#include "caf/type_id_list.hpp" #include "caf/type_id_list.hpp"
using namespace std::literals;
using namespace caf; using namespace caf;
#define STEP(message) \ #define STEP(message) \
MESSAGE(message); \ MESSAGE(message); \
if (true) if (true)
CAF_TEST(message builder can build messages incrermenetally) { SCENARIO("message builders can build messages incrementally") {
message_builder builder; GIVEN("a default-constructed message builder") {
CHECK(builder.empty()); WHEN("calling append and to_message multiple times") {
CHECK(builder.to_message().empty()); THEN("each message contains the values added so far") {
CHECK_EQ(builder.size(), 0u); message_builder builder;
STEP("after adding 1, the message is (1)") { CHECK(builder.empty());
builder.append(int32_t{1}); CHECK(builder.to_message().empty());
auto msg = builder.to_message(); CHECK_EQ(builder.size(), 0u);
CHECK_EQ(builder.size(), 1u); STEP("after adding 1, the message is (1)") {
CHECK_EQ(msg.types(), make_type_id_list<int32_t>()); builder.append(int32_t{1});
CHECK_EQ(to_string(msg.types()), "[int32_t]"); auto msg = builder.to_message();
CHECK_EQ(to_string(msg), "message(1)"); CHECK_EQ(builder.size(), 1u);
} CHECK_EQ(msg.types(), make_type_id_list<int32_t>());
STEP("after adding [2, 3], the message is (1, 2, 3)") { CHECK_EQ(to_string(msg.types()), "[int32_t]");
std::vector<int32_t> xs{2, 3}; CHECK_EQ(to_string(msg), "message(1)");
builder.append(xs.begin(), xs.end()); }
CHECK_EQ(builder.size(), 3u); STEP("after adding [2, 3], the message is (1, 2, 3)") {
auto msg = builder.to_message(); std::vector<int32_t> xs{2, 3};
CHECK_EQ(msg.types(), (make_type_id_list<int32_t, int32_t, int32_t>())); builder.append(xs.begin(), xs.end());
CHECK_EQ(to_string(msg.types()), "[int32_t, int32_t, int32_t]"); CHECK_EQ(builder.size(), 3u);
CHECK_EQ(to_string(msg), "message(1, 2, 3)"); auto msg = builder.to_message();
CHECK_EQ(msg.types(),
(make_type_id_list<int32_t, int32_t, int32_t>()));
CHECK_EQ(to_string(msg.types()), "[int32_t, int32_t, int32_t]");
CHECK_EQ(to_string(msg), "message(1, 2, 3)");
}
STEP("move_to_message produces the same message again") {
auto msg = builder.move_to_message();
CHECK_EQ(msg.types(),
(make_type_id_list<int32_t, int32_t, int32_t>()));
CHECK_EQ(to_string(msg.types()), "[int32_t, int32_t, int32_t]");
CHECK_EQ(to_string(msg), "message(1, 2, 3)");
}
}
}
} }
STEP("moving the content to a message produces the same message again") { }
auto msg = builder.move_to_message();
CHECK_EQ(msg.types(), (make_type_id_list<int32_t, int32_t, int32_t>())); SCENARIO("message builders allows RAII types") {
CHECK_EQ(to_string(msg.types()), "[int32_t, int32_t, int32_t]"); GIVEN("a default-constructed message builder") {
CHECK_EQ(to_string(msg), "message(1, 2, 3)"); WHEN("calling append with a string") {
THEN("to_message copies the string content into a message") {
message_builder builder;
std::string quote = "He who laughs at himself "
"never runs out of things to laugh at.";
builder.append(quote);
auto msg = builder.to_message();
CHECK_EQ(msg.types(), (make_type_id_list<std::string>()));
CHECK_EQ(to_string(msg.types()), "[std::string]");
using view_t = const_typed_message_view<std::string>;
if (auto tup = view_t(msg); CHECK(tup)) {
CHECK_EQ(get<0>(tup), quote);
}
}
}
} }
} }
...@@ -363,7 +363,7 @@ public: ...@@ -363,7 +363,7 @@ public:
expect_clause(expect_clause&& other) = default; expect_clause(expect_clause&& other) = default;
void eval(const char* type_str, const char* fields_str) { void eval(const char*, const char* fields_str) {
using namespace caf; using namespace caf;
test::logger::instance().verbose() test::logger::instance().verbose()
<< term::yellow << " -> " << term::reset << term::yellow << " -> " << term::reset
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment