From f9d9a0a09d8395243b0090348a2be75745dd0f5f Mon Sep 17 00:00:00 2001 From: rex-schilasky <49162693+rex-schilasky@users.noreply.github.com> Date: Thu, 12 Dec 2024 13:26:49 +0100 Subject: [PATCH] minor review adaptions --- ecal/core/src/service/ecal_service_client.cpp | 10 ----- .../src/service/ecal_service_client_impl.cpp | 4 +- ecal/core/src/service/ecal_service_server.cpp | 9 ---- .../src/service/ecal_service_server_impl.cpp | 41 +++++++++++++++---- 4 files changed, 34 insertions(+), 30 deletions(-) diff --git a/ecal/core/src/service/ecal_service_client.cpp b/ecal/core/src/service/ecal_service_client.cpp index a0d6e1e423..5398239533 100644 --- a/ecal/core/src/service/ecal_service_client.cpp +++ b/ecal/core/src/service/ecal_service_client.cpp @@ -58,23 +58,13 @@ namespace eCAL CServiceClient::CServiceClient(CServiceClient&& rhs) noexcept : m_service_client_impl(std::move(rhs.m_service_client_impl)) { - rhs.m_service_client_impl = nullptr; } CServiceClient& CServiceClient::operator=(CServiceClient&& rhs) noexcept { if (this != &rhs) { - // Unregister current client - if (g_clientgate()) - { - g_clientgate()->Unregister(m_service_client_impl->GetServiceName(), m_service_client_impl); - } - - // Move data m_service_client_impl = std::move(rhs.m_service_client_impl); - - rhs.m_service_client_impl = nullptr; } return *this; } diff --git a/ecal/core/src/service/ecal_service_client_impl.cpp b/ecal/core/src/service/ecal_service_client_impl.cpp index cd131ceafa..76860b2002 100644 --- a/ecal/core/src/service/ecal_service_client_impl.cpp +++ b/ecal/core/src/service/ecal_service_client_impl.cpp @@ -138,7 +138,7 @@ namespace eCAL m_event_callback = event_callback_; } - // register client + // Send registration sample if (!m_service_name.empty() && g_registration_provider() != nullptr) { g_registration_provider()->RegisterSample(GetRegistrationSample()); @@ -160,7 +160,7 @@ namespace eCAL m_event_callback = nullptr; } - // unregister client + // Send unregistration sample if (g_registration_provider() != nullptr) { g_registration_provider()->UnregisterSample(GetUnregistrationSample()); diff --git a/ecal/core/src/service/ecal_service_server.cpp b/ecal/core/src/service/ecal_service_server.cpp index 90b1e52a30..ce8a30b0e6 100644 --- a/ecal/core/src/service/ecal_service_server.cpp +++ b/ecal/core/src/service/ecal_service_server.cpp @@ -63,16 +63,7 @@ namespace eCAL { if (this != &rhs) { - // Unregister server - if (g_servicegate() != nullptr) - { - //g_servicegate()->Unregister(m_service_server_impl->GetServiceName(), m_service_server_impl); - } - - // Move data m_service_server_impl = std::move(rhs.m_service_server_impl); - - rhs.m_service_server_impl = nullptr; } return *this; } diff --git a/ecal/core/src/service/ecal_service_server_impl.cpp b/ecal/core/src/service/ecal_service_server_impl.cpp index 4206195599..bcec71c805 100644 --- a/ecal/core/src/service/ecal_service_server_impl.cpp +++ b/ecal/core/src/service/ecal_service_server_impl.cpp @@ -189,7 +189,7 @@ namespace eCAL return; } - // Register service + // Send registration sample if (g_registration_provider()) g_registration_provider()->RegisterSample(GetRegistrationSample()); @@ -207,6 +207,14 @@ namespace eCAL Logging::Log(log_level_debug1, "CServiceServerImpl: Stopping service server for: " + m_service_name); + // Stop TCP server + if (m_tcp_server) + { + m_tcp_server->stop(); + Logging::Log(log_level_debug1, "CServiceServerImpl: TCP server stopped for: " + m_service_name); + } + m_tcp_server.reset(); + // Reset method callbacks { std::lock_guard const lock(m_method_map_mutex); @@ -221,14 +229,7 @@ namespace eCAL Logging::Log(log_level_debug2, "CServiceServerImpl: Cleared event callback for: " + m_service_name); } - // Stop TCP server - if (m_tcp_server) - { - m_tcp_server->stop(); - Logging::Log(log_level_debug1, "CServiceServerImpl: TCP server stopped for: " + m_service_name); - } - - // Unregister service + // Send unregistration sample if (g_registration_provider()) g_registration_provider()->UnregisterSample(GetUnregistrationSample()); @@ -303,12 +304,14 @@ namespace eCAL { Logging::Log(log_level_debug2, "CServiceServerImpl: Processing request callback for: " + m_service_name); + // prepare response Service::Response response; auto& response_header = response.header; response_header.hname = Process::GetHostName(); response_header.sname = m_service_name; response_header.sid = m_service_id; + // try to parse request Service::Request request; if (!DeserializeFromBuffer(request_pb_.c_str(), request_pb_.size(), request)) { @@ -318,10 +321,16 @@ namespace eCAL std::string const emsg = "Service '" + m_service_name + "' request message could not be parsed."; response_header.error = emsg; + // 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) + // serialize response and return "request message could not be parsed" SerializeToBuffer(response, response_pb_); + + // Return Failed (error_code = -1), as parsing the request failed. The + // return value is not propagated to the remote caller. return -1; } + // get method SMethod method; const auto& request_header = request.header; response_header.mname = request_header.mname; @@ -330,11 +339,18 @@ namespace eCAL auto requested_method_iterator = m_method_map.find(request_header.mname); if (requested_method_iterator == m_method_map.end()) { + // set method call state 'failed' response_header.state = Service::eMethodCallState::failed; + // set error message std::string const emsg = "CServiceServerImpl: Service '" + m_service_name + "' has no method named '" + request_header.mname + "'"; response_header.error = emsg; + // 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) + // serialize response and return "method not found" SerializeToBuffer(response, response_pb_); + + // Return Success (error_code = 0), as parsing the request worked. The + // return value is not propagated to the remote caller. return 0; } else @@ -345,15 +361,22 @@ namespace eCAL } } + // execute method (outside lock guard) const std::string& request_s = request.request; std::string response_s; int const service_return_state = method.callback(method.method.mname, method.method.req_type, method.method.resp_type, request_s, response_s); + // set method call state 'executed' response_header.state = Service::eMethodCallState::executed; + // set method response and return state response.response = response_s; response.ret_state = service_return_state; + // 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) + // serialize response and return "method not found" SerializeToBuffer(response, response_pb_); + + // return success (error code 0) return 0; }