diff --git a/cpp/core/internal/base_pcp_handler.cc b/cpp/core/internal/base_pcp_handler.cc index 72e40e0e..c9fcb3bd 100644 --- a/cpp/core/internal/base_pcp_handler.cc +++ b/cpp/core/internal/base_pcp_handler.cc @@ -246,7 +246,7 @@ void BasePcpHandler::OnEncryptionSuccessRunnable( // Fail early, if there is no crypto context. ProcessPreConnectionInitiationFailure( endpoint_id, connection_info.channel.get(), {Status::kEndpointIoError}, - connection_info.result.get()); + connection_info.result.lock().get()); connection_info.result.reset(); return; } @@ -273,10 +273,10 @@ void BasePcpHandler::OnEncryptionSuccessRunnable( connection_info.options, std::move(connection_info.channel), connection_info.listener); - if (connection_info.result != nullptr) { + if (auto future_status = connection_info.result.lock()) { NEARBY_LOG(INFO, "Connection established; Finalising future OK"); - connection_info.result->Set({Status::kSuccess}); - connection_info.result = nullptr; + future_status->Set({Status::kSuccess}); + connection_info.result.reset(); } } @@ -307,7 +307,7 @@ void BasePcpHandler::OnEncryptionFailureRunnable( ProcessPreConnectionInitiationFailure(endpoint_id, info.channel.get(), {Status::kEndpointIoError}, - info.result.get()); + info.result.lock().get()); info.result.reset(); } @@ -315,15 +315,15 @@ Status BasePcpHandler::RequestConnection(ClientProxy* client, const std::string& endpoint_id, const ConnectionRequestInfo& info, const ConnectionOptions& options) { - Future result; - RunOnPcpHandlerThread([this, client, &info, options, endpoint_id, &result]() { + auto result = std::make_shared>(); + RunOnPcpHandlerThread([this, client, &info, options, endpoint_id, result]() { absl::Time start_time = SystemClock::ElapsedRealtime(); // If we already have a pending connection, then we shouldn't allow any more // outgoing connections to this endpoint. if (pending_connections_.count(endpoint_id)) { NEARBY_LOG(INFO, "Connection already exists: id=%s", endpoint_id.c_str()); - result.Set({Status::kAlreadyConnectedToEndpoint}); + result->Set({Status::kAlreadyConnectedToEndpoint}); return; } @@ -333,7 +333,7 @@ Status BasePcpHandler::RequestConnection(ClientProxy* client, !CanSendOutgoingConnection(client)) { NEARBY_LOG(INFO, "Outgoing connection not allowed: id=%s", endpoint_id.c_str()); - result.Set({Status::kOutOfOrderApiCall}); + result->Set({Status::kOutOfOrderApiCall}); return; } @@ -341,7 +341,7 @@ Status BasePcpHandler::RequestConnection(ClientProxy* client, if (endpoint == nullptr) { NEARBY_LOG(INFO, "Discovered endpoint not found: id=%s", endpoint_id.c_str()); - result.Set({Status::kEndpointUnknown}); + result->Set({Status::kEndpointUnknown}); return; } @@ -377,7 +377,7 @@ Status BasePcpHandler::RequestConnection(ClientProxy* client, NEARBY_LOG(INFO, "Endpoint channel not available: id=%s", endpoint_id.c_str()); ProcessPreConnectionInitiationFailure( - endpoint_id, channel.get(), connect_impl_result.status, &result); + endpoint_id, channel.get(), connect_impl_result.status, result.get()); return; } @@ -394,7 +394,7 @@ Status BasePcpHandler::RequestConnection(ClientProxy* client, NEARBY_LOG(INFO, "Failed to send connection request: id=%s", endpoint_id.c_str()); ProcessPreConnectionInitiationFailure( - endpoint_id, channel.get(), {Status::kEndpointIoError}, &result); + endpoint_id, channel.get(), {Status::kEndpointIoError}, result.get()); return; } @@ -417,7 +417,7 @@ Status BasePcpHandler::RequestConnection(ClientProxy* client, .start_time = start_time, .listener = info.listener, .options = options, - .result = MakeSwapper(&result), + .result = result, .channel = std::move(channel), }) .first->second.channel.get(); @@ -433,7 +433,7 @@ Status BasePcpHandler::RequestConnection(ClientProxy* client, endpoint_id.c_str()); auto status = WaitForResult(absl::StrCat("RequestConnection(", endpoint_id, ")"), - client->GetClientId(), &result); + client->GetClientId(), result.get()); NEARBY_LOG(INFO, "Wait is complete: id=%s; status=%d", endpoint_id.c_str(), status.value); return status; @@ -1010,8 +1010,8 @@ void BasePcpHandler::ProcessTieBreakLoss( BasePcpHandler::PendingConnectionInfo* info) { ProcessPreConnectionInitiationFailure(endpoint_id, info->channel.get(), {Status::kEndpointIoError}, - info->result.get()); - info->result = nullptr; + info->result.lock().get()); + info->result.reset(); ProcessPreConnectionResultFailure(client, endpoint_id); } @@ -1255,9 +1255,10 @@ ExceptionOr BasePcpHandler::ReadConnectionRequestFrame( ///////////////////// BasePcpHandler::PendingConnectionInfo /////////////////// BasePcpHandler::PendingConnectionInfo::~PendingConnectionInfo() { - if (result != nullptr) { + auto future_status = result.lock(); + if (future_status && !future_status->IsSet()) { NEARBY_LOG(INFO, "Future was not set; destroying info"); - result->Set({Status::kError}); + future_status->Set({Status::kError}); } if (channel != nullptr) { diff --git a/cpp/core/internal/base_pcp_handler.h b/cpp/core/internal/base_pcp_handler.h index 313b9cd5..a580ef83 100644 --- a/cpp/core/internal/base_pcp_handler.h +++ b/cpp/core/internal/base_pcp_handler.h @@ -40,36 +40,6 @@ namespace location { namespace nearby { namespace connections { -// Define a class that supports move operation for pointers using std::swap. -// It replicates std::unique_ptr<> behavior, but it does not own the pointer, -// so it does not attempt destroy it. -// This approach was recommended during code review, as a better alternative to -// reuse of std::unique_ptr<> with custom no-op deleter, for the sake of -// readability. -template -class Swapper { - public: - Swapper(T* pointer) : pointer_(pointer) {} // NOLINT. - Swapper(Swapper&& other) { *this = std::move(other); } - Swapper& operator=(Swapper&& other) { - std::swap(pointer_, other.pointer_); - return *this; - } - T* operator->() const { return pointer_; } - T& operator*() { return *pointer_; } - operator T*() { return pointer_; } // NOLINT. - T* get() const { return pointer_; } - void reset() { pointer_ = nullptr; } - - private: - T* pointer_ = nullptr; -}; - -template -Swapper MakeSwapper(T* value) { - return Swapper(value); -} - // Represents the WebRtc state that mediums are connectable or not. enum class WebRtcState { kUndefined = 0, @@ -341,7 +311,7 @@ class BasePcpHandler : public PcpHandler, // Only set for outgoing connections. If set, we must call // result->Set() when connection is established, or rejected. - Swapper> result = nullptr; + std::weak_ptr> result; // Only (possibly) vector for incoming connections. std::vector supported_mediums; diff --git a/cpp/core/internal/base_pcp_handler_test.cc b/cpp/core/internal/base_pcp_handler_test.cc index 2a9dffbf..ae8b5da6 100644 --- a/cpp/core/internal/base_pcp_handler_test.cc +++ b/cpp/core/internal/base_pcp_handler_test.cc @@ -157,18 +157,21 @@ class MockPcpHandler : public BasePcpHandler { class MockContext { public: - explicit MockContext(std::atomic_int* destroyed = nullptr) { - destroyed_ = destroyed; + explicit MockContext(std::atomic_int* destroyed = nullptr) + : destroyed_{destroyed} {} + MockContext(MockContext&& other) { *this = std::move(other); } + MockContext& operator=(MockContext&& other) { + destroyed_ = other.destroyed_; + other.destroyed_ = nullptr; + return *this; } - MockContext(MockContext&&) = default; - MockContext& operator=(MockContext&&) = default; ~MockContext() { if (destroyed_) (*destroyed_)++; } private: - Swapper destroyed_{nullptr}; + std::atomic_int* destroyed_; }; struct MockDiscoveredEndpoint : public MockPcpHandler::DiscoveredEndpoint {