From b7f01484244a0cfb8e98e293aadd0dd5759fe902 Mon Sep 17 00:00:00 2001 From: Florian Reimold <11774314+FlorianReimold@users.noreply.github.com> Date: Tue, 22 Aug 2023 08:18:02 +0200 Subject: [PATCH] Solved many TODOs --- ecal/core/src/ecal_globals.cpp | 2 - .../src/service/ecal_service_client_impl.cpp | 224 +----------------- .../src/service/ecal_service_client_impl.h | 8 - .../src/service/ecal_service_server_impl.cpp | 2 - .../src/service/ecal_service_server_impl.h | 12 +- ecal/service/CMakeLists.txt | 28 +-- .../include/ecal/service/client_session.h | 1 - ecal/service/src/client_session_impl_v0.cpp | 2 - ecal/service/src/client_session_impl_v1.cpp | 2 - .../src/ecal_tcp_service_test.cpp | 2 - 10 files changed, 16 insertions(+), 267 deletions(-) diff --git a/ecal/core/src/ecal_globals.cpp b/ecal/core/src/ecal_globals.cpp index 5e60e249e6..a433d12c3d 100644 --- a/ecal/core/src/ecal_globals.cpp +++ b/ecal/core/src/ecal_globals.cpp @@ -270,8 +270,6 @@ namespace eCAL // clientgate/servicegat. The callbacks in the service implementation carry // raw pointers to the gate's functions, so we must make sure that everything // has been executed, before we delete the gates. - // - // TODO: just make it a share pointer... it will be soooo much better. eCAL::service::ServiceManager::instance()->stop(); if (clientgate_instance) clientgate_instance->Destroy(); if (servicegate_instance) servicegate_instance->Destroy(); diff --git a/ecal/core/src/service/ecal_service_client_impl.cpp b/ecal/core/src/service/ecal_service_client_impl.cpp index 941595a8e4..f8a9621e78 100644 --- a/ecal/core/src/service/ecal_service_client_impl.cpp +++ b/ecal/core/src/service/ecal_service_client_impl.cpp @@ -196,7 +196,8 @@ namespace eCAL auto client = m_client_map.find(iter.key); if (client != m_client_map.end()) { - // Copy raw request in a protocol buffer TODO: This is kind of unnecessary and part of the protocol, so it also shouldn't be done here. + // Copy raw request in a protocol buffer + // TODO: The next version of the service protocol should omit the double-serialization (i.e. copying the binary data in a protocol buffer and then serializing that again) eCAL::pb::Request request_pb; request_pb.mutable_header()->set_mname(method_name_); request_pb.set_request(request_); @@ -232,7 +233,8 @@ namespace eCAL // check for new server CheckForNewServices(); - // Copy raw request in a protocol buffer TODO: This is kind of unnecessary and part of the protocol, so it also shouldn't be done here. + // Copy raw request in a protocol buffer + // TODO: The next version of the service protocol should omit the double-serialization (i.e. copying the binary data in a protocol buffer and then serializing that again) eCAL::pb::Request request_pb; request_pb.mutable_header()->set_mname(method_name_); request_pb.set_request(request_); @@ -490,7 +492,8 @@ namespace eCAL // check for new server CheckForNewServices(); - // Copy raw request in a protocol buffer TODO: This is kind of unnecessary and part of the protocol, so it also shouldn't be done here. + // Copy raw request in a protocol buffer + // TODO: The next version of the service protocol should omit the double-serialization (i.e. copying the binary data in a protocol buffer and then serializing that again) eCAL::pb::Request request_pb; request_pb.mutable_header()->set_mname(method_name_); request_pb.set_request(request_); @@ -510,7 +513,7 @@ namespace eCAL if (client != m_client_map.end()) { const eCAL::service::ClientResponseCallbackT response_callback - = [hostname = service.hname, servicename = service.sname, this] // TODO: using the this pointer here actually firces us to also manage the lifetime of this, e.g. with a shared ptr + = [hostname = service.hname, servicename = service.sname, this] // TODO: using the this pointer here actually forces us to also manage the lifetime of this, e.g. with a shared ptr (const eCAL::service::Error& response_error, const std::shared_ptr& response_) { std::lock_guard const lock(this->m_response_callback_sync); @@ -545,25 +548,6 @@ namespace eCAL } return(at_least_one_service_was_called); - - //================ Old Code. TODO: remove ============== - - //bool called(false); - //std::vector const service_vec = g_clientgate()->GetServiceAttr(m_service_name); - //for (const auto& iter : service_vec) - //{ - // if (m_host_name.empty() || (m_host_name == iter.hname)) - // { - // std::lock_guard const lock(m_client_map_sync); - // auto client = m_client_map.find(iter.key); - // if (client != m_client_map.end()) - // { - // SendRequestAsync(client->second, method_name_, request_ /*, timeout_*/, -1); - // called = true; - // } - // } - //} - //return(called); } // check connection state @@ -734,25 +718,12 @@ namespace eCAL auto client_manager = eCAL::service::ServiceManager::instance()->get_client_manager(); if (client_manager == nullptr || client_manager->is_stopped()) return; - // Event callback - // TODO: Make an actual implementation + // Event callback (unused) const eCAL::service::ClientSession::EventCallbackT event_callback - = [/*this, service_ = iter*/] // TODO: using the this pointer here is extremely unsafe, as it actually forces us to manage the lifetime of this object + = [/*this, service_ = iter*/] // Using the this pointer here is extremely unsafe, as it actually forces us to manage the lifetime of this object (eCAL::service::ClientEventType /*event*/, const std::string& /*message*/) -> void { - - // TODO: I have no idea why, but for some reason the event callbacks of the actual connetions are not even used. The connect / disconnect callbacks are executed whenever a new connection is found, and not when the client has actually connected or disconnected. I am preserving the previous behavior. - - //const std::lock_guard callback_map_lock(this->m_event_callback_map_sync); - //auto callback_it = this->m_event_callback_map.find(event); - //if (callback_it != m_event_callback_map.end()) - //{ - // SClientEventCallbackData sdata; - // sdata.type = event; - // sdata.time = std::chrono::duration_cast(std::chrono::steady_clock::now().time_since_epoch()).count(); - // sdata.attr = service_; - // (callback_it->second)(m_service_name.c_str(), &sdata); - //} + // I have no idea why, but for some reason the event callbacks of the actual connetions are not even used. The connect / disconnect callbacks are executed whenever a new connection is found, and not when the client has actually connected or disconnected. I am preserving the previous behavior. }; // Only connect via V0 protocol / V0 port, if V1 port is not available @@ -767,181 +738,6 @@ namespace eCAL } } - //bool CServiceClientImpl::SendRequests(const std::string& host_name_, const std::string& method_name_, const std::string& request_, int timeout_) - //{ - // if (g_clientgate() == nullptr) return false; - - // bool ret_state(false); - - // std::lock_guard const lock(m_client_map_sync); - // for (auto& client : m_client_map) - // { - // if (client.second->get_state() != eCAL::service::State::FAILED) - // { - // if (host_name_.empty() || (host_name_ == client.second->get_address())) - // { - // // execute request - // SServiceResponse service_response; - // ret_state = SendRequest(client.second, method_name_, request_, timeout_, service_response); - // if (!ret_state) - // { - // std::cerr << "CServiceClientImpl::SendRequests failed." << std::endl; - // } - // - // // call response callback - // if (service_response.call_state != call_state_none) - // { - // std::lock_guard const lock_cb(m_response_callback_sync); - // if (m_response_callback) m_response_callback(service_response); - // } - // else - // { - // // call_state_none means service no more available - // // we destroy the client here - // client.second->stop(); - - // // TODO: Remove the client from the map!?!? I think we currently keep a non-functional client here. - // } - // // collect return state - // ret_state = true; - // } - // } - // } - // return ret_state; - //} - - //bool CServiceClientImpl::SendRequest(const std::shared_ptr& client_, const std::string& method_name_, const std::string& request_, int timeout_, struct SServiceResponse& service_response_) - //{ - // // create request protocol buffer - // eCAL::pb::Request request_pb; - // request_pb.mutable_header()->set_mname(method_name_); - // request_pb.set_request(request_); - // std::string const request_s = request_pb.SerializeAsString(); - - // // catch events - // client_->AddEventCallback([this](eCAL_Client_Event event, const std::string& /*message*/) - // { - // switch (event) - // { - // case client_event_timeout: - // { - // std::lock_guard const lock_eb(m_event_callback_map_sync); - // auto e_iter = m_event_callback_map.find(client_event_timeout); - // if (e_iter != m_event_callback_map.end()) - // { - // SClientEventCallbackData sdata; - // sdata.type = client_event_timeout; - // sdata.time = std::chrono::duration_cast(std::chrono::steady_clock::now().time_since_epoch()).count(); - // (e_iter->second)(m_service_name.c_str(), &sdata); - // } - // } - // break; - // default: - // break; - // } - // }); - - // // execute request - // std::string response_s; - // size_t const sent = client_->ExecuteRequest(request_s, timeout_, response_s); - // if (sent == 0) return false; - - // // parse response protocol buffer - // eCAL::pb::Response response_pb; - // if (!response_pb.ParseFromString(response_s)) - // { - // std::cerr << "CServiceClientImpl::SendRequest Could not parse server response !" << std::endl; - // return false; - // } - - // const auto& response_pb_header = response_pb.header(); - // service_response_.host_name = response_pb_header.hname(); - // service_response_.service_name = response_pb_header.sname(); - // service_response_.service_id = response_pb_header.sid(); - // service_response_.method_name = response_pb_header.mname(); - // service_response_.error_msg = response_pb_header.error(); - // service_response_.ret_state = static_cast(response_pb.ret_state()); - // switch (response_pb_header.state()) - // { - // case eCAL::pb::ServiceHeader_eCallState_executed: - // service_response_.call_state = call_state_executed; - // break; - // case eCAL::pb::ServiceHeader_eCallState_failed: - // service_response_.call_state = call_state_failed; - // break; - // default: - // break; - // } - // service_response_.response = response_pb.response(); - - // return (service_response_.call_state == call_state_executed); - //} - - //void CServiceClientImpl::SendRequestAsync(const std::shared_ptr& client_, const std::string& method_name_, const std::string& request_, int timeout_) - //{ - // // create request protocol buffer - // eCAL::pb::Request request_pb; - // request_pb.mutable_header()->set_mname(method_name_); - // request_pb.set_request(request_); - // std::string const request_s = request_pb.SerializeAsString(); - - // client_->ExecuteRequestAsync(request_s, timeout_, [this, client_, method_name_](const std::string& response, bool success) - // { - // std::lock_guard const lock(m_response_callback_sync); - // if (m_response_callback) - // { - // SServiceResponse service_response; - // if (!success) - // { - // const auto *error_msg = "CServiceClientImpl::SendRequestAsync failed !"; - // service_response.call_state = call_state_failed; - // service_response.error_msg = error_msg; - // service_response.ret_state = 0; - // service_response.method_name = method_name_; - // service_response.response.clear(); - // m_response_callback(service_response); - // return; - // } - - // eCAL::pb::Response response_pb; - // if (!response_pb.ParseFromString(response)) - // { - // const auto *error_msg = "CServiceClientImpl::SendRequestAsync could not parse server response !"; - // std::cerr << error_msg << "\n"; - // service_response.call_state = call_state_failed; - // service_response.error_msg = error_msg; - // service_response.ret_state = 0; - // service_response.method_name = method_name_; - // service_response.response.clear(); - // m_response_callback(service_response); - // return; - // } - - // const auto& response_pb_header = response_pb.header(); - // service_response.host_name = response_pb_header.hname(); - // service_response.service_name = response_pb_header.sname(); - // service_response.service_id = response_pb_header.sid(); - // service_response.method_name = response_pb_header.mname(); - // service_response.error_msg = response_pb_header.error(); - // service_response.ret_state = static_cast(response_pb.ret_state()); - // switch (response_pb_header.state()) - // { - // case eCAL::pb::ServiceHeader_eCallState_executed: - // service_response.call_state = call_state_executed; - // break; - // case eCAL::pb::ServiceHeader_eCallState_failed: - // service_response.call_state = call_state_failed; - // break; - // default: - // break; - // } - // service_response.response = response_pb.response(); - - // m_response_callback(service_response); - // } - // }); - //} - void CServiceClientImpl::ErrorCallback(const std::string& method_name_, const std::string& error_message_) { std::lock_guard const lock(m_response_callback_sync); diff --git a/ecal/core/src/service/ecal_service_client_impl.h b/ecal/core/src/service/ecal_service_client_impl.h index 257a37a324..bfba3afe32 100644 --- a/ecal/core/src/service/ecal_service_client_impl.h +++ b/ecal/core/src/service/ecal_service_client_impl.h @@ -95,19 +95,11 @@ namespace eCAL static void fromSerializedProtobuf(const std::string& response_pb_string, eCAL::SServiceResponse& response); static void fromProtobuf (const eCAL::pb::Response& response_pb, eCAL::SServiceResponse& response); - // TODO: Why is all this stuff protected???? Shouldn't this be private? - protected: void Register(bool force_); void Unregister(); void CheckForNewServices(); - // TODO: Remove - //bool SendRequests(const std::string& host_name_, const std::string& method_name_, const std::string& request_, int timeout_); - //bool SendRequest(const std::shared_ptr& client_, const std::string& method_name_, const std::string& request_, int timeout_, struct SServiceResponse& service_response_); - - //void SendRequestAsync(const std::shared_ptr& client_, const std::string& method_name_, const std::string& request_, int timeout_); - void ErrorCallback(const std::string &method_name_, const std::string &error_message_); using ClientMapT = std::map>; diff --git a/ecal/core/src/service/ecal_service_server_impl.cpp b/ecal/core/src/service/ecal_service_server_impl.cpp index f06b040f8b..5aa1b104a3 100644 --- a/ecal/core/src/service/ecal_service_server_impl.cpp +++ b/ecal/core/src/service/ecal_service_server_impl.cpp @@ -72,7 +72,6 @@ namespace eCAL // Get global server manager auto server_manager = eCAL::service::ServiceManager::instance()->get_server_manager(); - // TODO: Check for nullptr if (!server_manager || server_manager->is_stopped()) return false; @@ -105,7 +104,6 @@ namespace eCAL = [weak_me = std::weak_ptr(shared_from_this())] (const std::shared_ptr& request, const std::shared_ptr& response) -> int { - // TODO: I can change the signature of the RequestCallback to use shared_ptr auto me = weak_me.lock(); if (me) return me->RequestCallback(*request, *response); diff --git a/ecal/core/src/service/ecal_service_server_impl.h b/ecal/core/src/service/ecal_service_server_impl.h index bba53059a3..331b0c1b3c 100644 --- a/ecal/core/src/service/ecal_service_server_impl.h +++ b/ecal/core/src/service/ecal_service_server_impl.h @@ -53,15 +53,13 @@ namespace eCAL private: CServiceServerImpl(); - // Delete copy and move constructors and assign operators - CServiceServerImpl(const CServiceServerImpl&) = delete; // Copy construct - CServiceServerImpl(CServiceServerImpl&&) = delete; // Move construct - CServiceServerImpl& operator=(const CServiceServerImpl&) = delete; // Copy assign - CServiceServerImpl& operator=(CServiceServerImpl&&) = delete; // Move assign - public: + // Delete copy and move constructors and assign operators. Necessary, as the class uses the this pointer, that would be dangling / pointing to a wrong object otherwise. + CServiceServerImpl(const CServiceServerImpl&) = delete; // Copy construct + CServiceServerImpl(CServiceServerImpl&&) = delete; // Move construct + CServiceServerImpl& operator=(const CServiceServerImpl&) = delete; // Copy assign + CServiceServerImpl& operator=(CServiceServerImpl&&) = delete; // Move assign - // TODO: The this pointer is used internally, so this class must not be copyable or movable. ~CServiceServerImpl(); diff --git a/ecal/service/CMakeLists.txt b/ecal/service/CMakeLists.txt index 604b0e3584..f7e035d262 100644 --- a/ecal/service/CMakeLists.txt +++ b/ecal/service/CMakeLists.txt @@ -1,9 +1,5 @@ cmake_minimum_required(VERSION 3.5.1) -# TODO: Add a version number later -#include("${CMAKE_CURRENT_LIST_DIR}/version.cmake") -#project(ecal_service VERSION ${ECAL_SERVICE_VERSION_MAJOR}.${ECAL_SERVICE_VERSION_MINOR}.${ECAL_SERVICE_VERSION_PATCH}) - project(ecal_service) set(CMAKE_POSITION_INDEPENDENT_CODE ON) @@ -15,10 +11,6 @@ set(CMAKE_VISIBILITY_INLINES_HIDDEN 1) find_package(Threads REQUIRED) find_package(asio REQUIRED) -# TODO: generate export header doesn't work with object libs. Is there any solution? -# Include GenerateExportHeader that will create export macros for us -#include(GenerateExportHeader) - # Public API include directory set (includes include/ecal/service/client_manager.h @@ -60,26 +52,12 @@ set(sources src/server_session_impl_v1.h ) -#TODO: Make it variable to build obj/static/shared +# Build as object library add_library (${PROJECT_NAME} OBJECT ${includes} ${sources} ) -# TODO: Add the version later, again -# Generate version defines -#configure_file("ecal_service_version.h.in" "${PROJECT_BINARY_DIR}/include/ecal_service/ecal_service_version.h" @ONLY) - -# TODO: generate export header doesn't work with object libs. Is there any solution? -# Generate header with export macros -#generate_export_header(${PROJECT_NAME} -# EXPORT_FILE_NAME ${PROJECT_BINARY_DIR}/include/ecal/service/ecal_service_export.h -# BASE_NAME ECAL_SERVICE -#) - -# TODO: review this alias target. It probably should be eCAL::Service or something like that. -# add_library (ecal_service::${PROJECT_NAME} ALIAS ${PROJECT_NAME}) - target_link_libraries(${PROJECT_NAME} PUBLIC # Link header-only libs (asio & recycle) as described in this workaround: @@ -111,16 +89,12 @@ target_compile_options(${PROJECT_NAME} PRIVATE target_include_directories(${PROJECT_NAME} PUBLIC $ - $ # To find the export file generated by generate_export_header $ PRIVATE src/ ) set_target_properties(${PROJECT_NAME} PROPERTIES -# TODO: Add the version later again -# VERSION ${PROJECT_VERSION} -# SOVERSION ${PROJECT_VERSION_MAJOR} OUTPUT_NAME ${PROJECT_NAME} FOLDER ecal/core ) diff --git a/ecal/service/include/ecal/service/client_session.h b/ecal/service/include/ecal/service/client_session.h index 16df21bffc..9c658b423e 100644 --- a/ecal/service/include/ecal/service/client_session.h +++ b/ecal/service/include/ecal/service/client_session.h @@ -56,7 +56,6 @@ namespace eCAL // Constructor, Destructor, Create ////////////////////////////////////////////// public: - // TODO: Maybe I can remove all those overloads and just keep 1 static std::shared_ptr create(const std::shared_ptr& io_context , std::uint8_t protocol_version , const std::string& address diff --git a/ecal/service/src/client_session_impl_v0.cpp b/ecal/service/src/client_session_impl_v0.cpp index eebc3199d1..e39be517cc 100644 --- a/ecal/service/src/client_session_impl_v0.cpp +++ b/ecal/service/src/client_session_impl_v0.cpp @@ -73,8 +73,6 @@ namespace eCAL ////////////////////////////////////// void ClientSessionV0::resolve_endpoint() { - // TODO: Should I create a localhost shortcut here? One that hardcoded goes to 127.0.0.1? Windows does not support localhost resolving, so this would be a workaround. - ECAL_SERVICE_LOG_DEBUG(logger_, "Resolving endpoint [" + address_ + ":" + std::to_string(port_) + "]..."); const asio::ip::tcp::resolver::query query(address_, std::to_string(port_)); diff --git a/ecal/service/src/client_session_impl_v1.cpp b/ecal/service/src/client_session_impl_v1.cpp index 4e88fe1d6f..b950b1df7e 100644 --- a/ecal/service/src/client_session_impl_v1.cpp +++ b/ecal/service/src/client_session_impl_v1.cpp @@ -77,8 +77,6 @@ namespace eCAL ////////////////////////////////////// void ClientSessionV1::resolve_endpoint() { - // TODO: Should I create a localhost shortcut here? One that hardcoded goes to 127.0.0.1? Windows does not support localhost resolving, so this would be a workaround. - ECAL_SERVICE_LOG_DEBUG(logger_, "Resolving endpoint [" + address_ + ":" + std::to_string(port_) + "]..."); const asio::ip::tcp::resolver::query query(address_, std::to_string(port_)); diff --git a/testing/ecal/service_test/src/ecal_tcp_service_test.cpp b/testing/ecal/service_test/src/ecal_tcp_service_test.cpp index f6e14c025e..86d3b94cfe 100644 --- a/testing/ecal/service_test/src/ecal_tcp_service_test.cpp +++ b/testing/ecal/service_test/src/ecal_tcp_service_test.cpp @@ -2505,6 +2505,4 @@ TEST(BlockingCall, Stopped) // NOLINT // TODO: This test shows the proper way t } #endif -// TODO: Test blocking call when the io_context is stopped and then a call is made - // TODO: Add a test for single-threaded service callbacks (-> setting parallel_service_callbacks to false)