Skip to content

Commit befb550

Browse files
committed
CXXCBC-618: fix memory leak and data race
using std::function along with std::shared_ptr for promise might affect the lifetime of the cluster object and result in memory leak and data races during destruction
1 parent c612b9c commit befb550

File tree

5 files changed

+41
-29
lines changed

5 files changed

+41
-29
lines changed

core/impl/cluster.cxx

+33-20
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ class cluster_impl : public std::enable_shared_from_this<cluster_impl>
240240

241241
void open(const std::string& connection_string,
242242
const cluster_options& options,
243-
cluster_connect_handler&& handler)
243+
core::utils::movable_function<void(error, cluster)>&& handler)
244244
{
245245
core_.open(
246246
options_to_origin(connection_string, options),
@@ -512,39 +512,52 @@ cluster::search(std::string index_name,
512512
return barrier->get_future();
513513
}
514514

515-
auto
516-
cluster::connect(const std::string& connection_string,
517-
const cluster_options& options) -> std::future<std::pair<error, cluster>>
515+
namespace
518516
{
519-
auto barrier = std::make_shared<std::promise<std::pair<error, cluster>>>();
520-
auto future = barrier->get_future();
521-
connect(connection_string, options, [barrier](auto err, auto c) {
522-
barrier->set_value({ std::move(err), std::move(c) });
523-
});
524-
return future;
525-
}
526-
527517
void
528-
cluster::connect(const std::string& connection_string,
529-
const cluster_options& options,
530-
cluster_connect_handler&& handler)
518+
connect_in_background(const std::string& connection_string,
519+
const cluster_options& options,
520+
core::utils::movable_function<void(error, cluster)>&& handler)
531521
{
532522
// Spawn new thread for connection to ensure that cluster_impl pointer will
533523
// not be deallocated in IO thread in case of error.
534524
std::thread([connection_string, options, handler = std::move(handler)]() {
535-
auto barrier = std::make_shared<std::promise<std::pair<error, cluster>>>();
536-
auto future = barrier->get_future();
525+
std::promise<std::pair<error, cluster>> barrier;
526+
auto future = barrier.get_future();
537527
{
538528
auto impl = std::make_shared<cluster_impl>();
539-
impl->open(connection_string, options, [barrier](auto err, auto c) {
540-
barrier->set_value({ std::move(err), std::move(c) });
541-
});
529+
impl->open(
530+
connection_string, options, [barrier = std::move(barrier)](auto err, auto c) mutable {
531+
barrier.set_value({ std::move(err), std::move(c) });
532+
});
542533
}
543534

544535
auto [err, c] = future.get();
545536
handler(std::move(err), std::move(c));
546537
}).detach();
547538
}
539+
} // namespace
540+
541+
auto
542+
cluster::connect(const std::string& connection_string,
543+
const cluster_options& options) -> std::future<std::pair<error, cluster>>
544+
{
545+
std::promise<std::pair<error, cluster>> barrier;
546+
auto future = barrier.get_future();
547+
connect_in_background(
548+
connection_string, options, [barrier = std::move(barrier)](auto err, auto c) mutable {
549+
barrier.set_value({ std::move(err), std::move(c) });
550+
});
551+
return future;
552+
}
553+
554+
void
555+
cluster::connect(const std::string& connection_string,
556+
const cluster_options& options,
557+
cluster_connect_handler&& handler)
558+
{
559+
connect_in_background(connection_string, options, std::move(handler));
560+
}
548561

549562
auto
550563
cluster::notify_fork(fork_event event) -> void

core/io/http_command.hxx

+2-2
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,13 @@ struct http_command : public std::enable_shared_from_this<http_command<Request>>
4949
asio::steady_timer deadline;
5050
Request request;
5151
encoded_request_type encoded;
52-
std::shared_ptr<tracing::tracer_wrapper> tracer_;
52+
std::shared_ptr<tracing::tracer_wrapper> tracer_{ nullptr };
5353
std::shared_ptr<couchbase::tracing::request_span> span_{ nullptr };
5454
std::shared_ptr<metrics::meter_wrapper> meter_{};
5555
std::shared_ptr<io::http_session> session_{};
5656
http_command_handler handler_{};
5757
std::chrono::milliseconds timeout_{};
58-
std::string client_context_id_;
58+
std::string client_context_id_{};
5959
std::shared_ptr<couchbase::tracing::request_span> parent_span{ nullptr };
6060
#ifdef COUCHBASE_CXX_CLIENT_COLUMNAR
6161
std::chrono::milliseconds dispatch_timeout_{};

core/io/http_message.hxx

+2-2
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@ struct streaming_settings {
3636
};
3737

3838
struct http_request {
39-
service_type type;
39+
service_type type{};
4040
std::string method{};
41-
std::string path;
41+
std::string path{};
4242
std::map<std::string, std::string> headers{};
4343
std::string body{};
4444
std::optional<streaming_settings> streaming{};

core/io/http_session_manager.hxx

+1-2
Original file line numberDiff line numberDiff line change
@@ -208,8 +208,7 @@ public:
208208
std::scoped_lock lock(sessions_mutex_);
209209
busy_sessions_[type].push_back(session);
210210
}
211-
operations::http_noop_request request{};
212-
request.type = type;
211+
operations::http_noop_request request{ type };
213212
request.timeout = timeout;
214213
#ifdef COUCHBASE_CXX_CLIENT_COLUMNAR
215214
auto cmd = std::make_shared<operations::http_command<operations::http_noop_request>>(

core/metrics/meter_wrapper.hxx

+3-3
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,9 @@
3333
namespace couchbase::core::metrics
3434
{
3535
struct metric_attributes {
36-
couchbase::core::service_type service;
37-
std::string operation;
38-
std::error_code ec;
36+
couchbase::core::service_type service{};
37+
std::string operation{};
38+
std::error_code ec{};
3939
std::optional<std::string> bucket_name{};
4040
std::optional<std::string> scope_name{};
4141
std::optional<std::string> collection_name{};

0 commit comments

Comments
 (0)