From 071e38c9dea526b1d712c32ea684c873894d98d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stanislav=20Angelovi=C4=8D?= Date: Tue, 24 Oct 2023 21:56:47 +0200 Subject: [PATCH] refactor: let callbacks take message objects by value (#367) Signatures of callbacks async_reply_handler, signal_handler, message_handler and property_set_callback were modified to take input message objects by value, as opposed to non-const ref. The callee assumes ownership of the message. This API is more idiomatic, more expressive, cleaner and safer. Move semantics is used to pass messages to the callback handlers. In some cases, this also improves performance. --- ChangeLog | 18 ++++++++++-------- docs/using-sdbus-c++.md | 6 +++--- include/sdbus-c++/ConvenienceApiClasses.inl | 6 +++--- include/sdbus-c++/IConnection.h | 2 +- include/sdbus-c++/TypeTraits.h | 8 ++++---- src/Connection.cpp | 4 ++-- src/Object.cpp | 4 ++-- src/Proxy.cpp | 10 +++++----- src/Proxy.h | 2 +- tests/integrationtests/DBusGeneralTests.cpp | 12 ++++++------ 10 files changed, 37 insertions(+), 35 deletions(-) diff --git a/ChangeLog b/ChangeLog index d74337b..65841db 100644 --- a/ChangeLog +++ b/ChangeLog @@ -242,14 +242,16 @@ v1.4.0 v2.x - Breaking changes in API/ABI/behavior: - - In *synchronous* D-Bus calls, the proxy now **always** blocks the connection for concurrent use until the call finishes (with either a received reply, - an error, or time out). If this creates a connection contention issue in your multi-threaded design, use short-lived, light-weight proxies, or call - the method in an asynchronous way. - - The `PollData` struct has been extended with a new data member: `eventFd`. All hooks with external event loops shall be modified to poll on this fd as well. - - `PollData::timeout_usec` was renamed to `PollData::timeout` and its type has been changed to `std::chrono::microseconds`. This member now holds directly - what before had to be obtained through `PollData::getAbsoluteTimeout()` call. - - `PollData::getRelativeTimeout()` return type was changed to `std::chrono::microseconds`. - - `IConnection::processPendingRequest()` was renamed to `IConnection::processPendingEvent()`. + - In *synchronous* D-Bus calls, the proxy now **always** blocks the connection for concurrent use until the call finishes (with either a received reply, + an error, or time out). If this creates a connection contention issue in your multi-threaded design, use short-lived, light-weight proxies, or call + the method in an asynchronous way. + - Signatures of callbacks `async_reply_handler`, `signal_handler`, `message_handler` and `property_set_callback` were modified to take input message objects + by value, as opposed to non-const ref. Callee assumes ownership of the message. This API is more idiomatic, cleaner and safer. + - The `PollData` struct has been extended with a new data member: `eventFd`. All hooks with external event loops shall be modified to poll on this fd as well. + - `PollData::timeout_usec` was renamed to `PollData::timeout` and its type has been changed to `std::chrono::microseconds`. This member now holds directly + what before had to be obtained through `PollData::getAbsoluteTimeout()` call. + - `PollData::getRelativeTimeout()` return type was changed to `std::chrono::microseconds`. + - `IConnection::processPendingRequest()` was renamed to `IConnection::processPendingEvent()`. - Systemd of at least v238 is required to compile sdbus-c++ - A proper fix for timeout handling - Fix for external event loops in which the event loop thread ID was not correctly initialized (now fixed and simplified by not needing the thread ID anymore) diff --git a/docs/using-sdbus-c++.md b/docs/using-sdbus-c++.md index 8e4f947..d771402 100644 --- a/docs/using-sdbus-c++.md +++ b/docs/using-sdbus-c++.md @@ -330,7 +330,7 @@ Please note that we can create and destroy D-Bus objects on a connection dynamic #include #include -void onConcatenated(sdbus::Signal& signal) +void onConcatenated(sdbus::Signal signal) { std::string concatenatedString; signal >> concatenatedString; @@ -389,7 +389,7 @@ int main(int argc, char *argv[]) In simple cases, we don't need to create D-Bus connection explicitly for our proxies. Unless a connection is provided to a proxy object explicitly via factory parameter, the proxy will create a connection of his own (unless it is a light-weight, short-lived proxy created with `dont_run_event_loop_thread_t`), and it will be a system bus connection. This is the case in the example above. (This approach is not scalable and resource-saving if we have plenty of proxies; see section [Working with D-Bus connections](#working-with-d-bus-connections-in-sdbus-c) for elaboration.) So, in the example, we create a proxy for object `/org/sdbuscpp/concatenator` publicly available at bus `org.sdbuscpp.concatenator`. We register signal handlers, if any, and finish the registration, making the proxy ready for use. -The callback for a D-Bus signal handler on this level is any callable of signature `void(sdbus::Signal& signal)`. The one and only parameter `signal` is the incoming signal message. We need to deserialize arguments from it, and then we can do our business logic with it. +The callback for a D-Bus signal handler on this level is any callable of signature `void(sdbus::Signal signal)`. The one and only parameter `signal` is the incoming signal message. We need to deserialize arguments from it, and then we can do our business logic with it. Subsequently, we invoke two RPC calls to object's `concatenate()` method. We create a method call message by invoking proxy's `createMethodCall()`. We serialize method input arguments into it, and make a synchronous call via proxy's `callMethod()`. As a return value we get the reply message as soon as it arrives. We deserialize return values from that message, and further use it in our program. The second `concatenate()` RPC call is done with invalid arguments, so we get a D-Bus error reply from the service, which as we can see is manifested via `sdbus::Error` exception being thrown. @@ -1117,7 +1117,7 @@ int main(int argc, char *argv[]) { /* ... */ - auto callback = [](MethodReply& reply, const sdbus::Error* error) + auto callback = [](MethodReply reply, const sdbus::Error* error) { if (error == nullptr) // No error { diff --git a/include/sdbus-c++/ConvenienceApiClasses.inl b/include/sdbus-c++/ConvenienceApiClasses.inl index f84dc93..0dd883c 100644 --- a/include/sdbus-c++/ConvenienceApiClasses.inl +++ b/include/sdbus-c++/ConvenienceApiClasses.inl @@ -317,7 +317,7 @@ namespace sdbus { if (propertySignature_.empty()) propertySignature_ = signature_of_function_input_arguments<_Function>::str(); - setter_ = [callback = std::forward<_Function>(callback)](PropertySetCall& call) + setter_ = [callback = std::forward<_Function>(callback)](PropertySetCall call) { // Default-construct property value using property_type = function_argument_t<_Function, 0>; @@ -580,7 +580,7 @@ namespace sdbus { { assert(method_.isValid()); // onInterface() must be placed/called prior to this function - auto asyncReplyHandler = [callback = std::forward<_Function>(callback)](MethodReply& reply, const Error* error) + auto asyncReplyHandler = [callback = std::forward<_Function>(callback)](MethodReply reply, const Error* error) { // Create a tuple of callback input arguments' types, which will be used // as a storage for the argument values deserialized from the message. @@ -656,7 +656,7 @@ namespace sdbus { proxy_.registerSignalHandler( interfaceName_ , signalName_ - , [callback = std::forward<_Function>(callback)](Signal& signal) + , [callback = std::forward<_Function>(callback)](Signal signal) { // Create a tuple of callback input arguments' types, which will be used // as a storage for the argument values deserialized from the signal message. diff --git a/include/sdbus-c++/IConnection.h b/include/sdbus-c++/IConnection.h index 77a79f0..bb6e59c 100644 --- a/include/sdbus-c++/IConnection.h +++ b/include/sdbus-c++/IConnection.h @@ -286,7 +286,7 @@ namespace sdbus { * The syntax of the match rule expression passed in match is described in the D-Bus specification. * The specified handler function callback is called for each incoming message matching the specified * expression. The match is installed synchronously when connected to a bus broker, i.e. the call - * sends a control message requested the match to be added to the broker and waits until the broker + * sends a control message requesting the match to be added to the broker and waits until the broker * confirms the match has been installed successfully. * * Simply let go of the slot instance to uninstall the match rule from the bus connection. The slot diff --git a/include/sdbus-c++/TypeTraits.h b/include/sdbus-c++/TypeTraits.h index 1de2ef4..acd8607 100644 --- a/include/sdbus-c++/TypeTraits.h +++ b/include/sdbus-c++/TypeTraits.h @@ -62,10 +62,10 @@ namespace sdbus { // Callbacks from sdbus-c++ using method_callback = std::function; - using async_reply_handler = std::function; - using signal_handler = std::function; - using message_handler = std::function; - using property_set_callback = std::function; + using async_reply_handler = std::function; + using signal_handler = std::function; + using message_handler = std::function; + using property_set_callback = std::function; using property_get_callback = std::function; // Type-erased RAII-style handle to callbacks/subscriptions registered to sdbus-c++ diff --git a/src/Connection.cpp b/src/Connection.cpp index 3b8a470..9099a6a 100644 --- a/src/Connection.cpp +++ b/src/Connection.cpp @@ -811,7 +811,7 @@ int Connection::sdbus_match_callback(sd_bus_message *sdbusMessage, void *userDat { auto* matchInfo = static_cast(userData); auto message = Message::Factory::create(sdbusMessage, &matchInfo->connection.getSdBusInterface()); - auto ok = invokeHandlerAndCatchErrors([&](){ matchInfo->callback(message); }, retError); + auto ok = invokeHandlerAndCatchErrors([&](){ matchInfo->callback(std::move(message)); }, retError); return ok ? 0 : -1; } @@ -819,7 +819,7 @@ int Connection::sdbus_match_install_callback(sd_bus_message *sdbusMessage, void { auto* matchInfo = static_cast(userData); auto message = Message::Factory::create(sdbusMessage, &matchInfo->connection.getSdBusInterface()); - auto ok = invokeHandlerAndCatchErrors([&](){ matchInfo->installCallback(message); }, retError); + auto ok = invokeHandlerAndCatchErrors([&](){ matchInfo->installCallback(std::move(message)); }, retError); return ok ? 0 : -1; } diff --git a/src/Object.cpp b/src/Object.cpp index d972195..8cefe61 100644 --- a/src/Object.cpp +++ b/src/Object.cpp @@ -341,7 +341,7 @@ int Object::sdbus_method_callback(sd_bus_message *sdbusMessage, void *userData, auto& callback = interfaceData->methods[message.getMemberName()].callback; assert(callback); - auto ok = invokeHandlerAndCatchErrors([&](){ callback(message); }, retError); + auto ok = invokeHandlerAndCatchErrors([&](){ callback(std::move(message)); }, retError); return ok ? 1 : -1; } @@ -390,7 +390,7 @@ int Object::sdbus_property_set_callback( sd_bus */*bus*/ auto value = Message::Factory::create(sdbusValue, &object.connection_.getSdBusInterface()); - auto ok = invokeHandlerAndCatchErrors([&](){ callback(value); }, retError); + auto ok = invokeHandlerAndCatchErrors([&](){ callback(std::move(value)); }, retError); return ok ? 1 : -1; } diff --git a/src/Proxy.cpp b/src/Proxy.cpp index a2826c6..856dffc 100644 --- a/src/Proxy.cpp +++ b/src/Proxy.cpp @@ -119,10 +119,10 @@ std::future Proxy::callMethod(const MethodCall& message, uint64_t t auto promise = std::make_shared>(); auto future = promise->get_future(); - async_reply_handler asyncReplyCallback = [promise = std::move(promise)](MethodReply& reply, const Error* error) noexcept + async_reply_handler asyncReplyCallback = [promise = std::move(promise)](MethodReply reply, const Error* error) noexcept { if (error == nullptr) - promise->set_value(reply); + promise->set_value(std::move(reply)); else promise->set_exception(std::make_exception_ptr(*error)); }; @@ -230,12 +230,12 @@ int Proxy::sdbus_async_reply_handler(sd_bus_message *sdbusMessage, void *userDat const auto* error = sd_bus_message_get_error(sdbusMessage); if (error == nullptr) { - asyncCallData->callback(message, nullptr); + asyncCallData->callback(std::move(message), nullptr); } else { Error exception(error->name, error->message); - asyncCallData->callback(message, &exception); + asyncCallData->callback(std::move(message), &exception); } }, retError); @@ -250,7 +250,7 @@ int Proxy::sdbus_signal_handler(sd_bus_message *sdbusMessage, void *userData, sd auto message = Message::Factory::create(sdbusMessage, &signalData->proxy.connection_->getSdBusInterface()); - auto ok = invokeHandlerAndCatchErrors([&](){ signalData->callback(message); }, retError); + auto ok = invokeHandlerAndCatchErrors([&](){ signalData->callback(std::move(message)); }, retError); return ok ? 0 : -1; } diff --git a/src/Proxy.h b/src/Proxy.h index af45d63..ab1d7ad 100644 --- a/src/Proxy.h +++ b/src/Proxy.h @@ -119,7 +119,7 @@ namespace sdbus::internal { public: struct CallData { - enum class State + enum class State // TODO: In release/v2.0, we no more have sync-in-terms-of-async, we can simplify code with just bool finished like before { NOT_ASYNC , RUNNING , FINISHED diff --git a/tests/integrationtests/DBusGeneralTests.cpp b/tests/integrationtests/DBusGeneralTests.cpp index 17816c0..c7b318b 100644 --- a/tests/integrationtests/DBusGeneralTests.cpp +++ b/tests/integrationtests/DBusGeneralTests.cpp @@ -76,7 +76,7 @@ TYPED_TEST(AConnection, WillCallCallbackHandlerForIncomingMessageMatchingMatchRu { auto matchRule = "sender='" + BUS_NAME + "',path='" + OBJECT_PATH + "'"; std::atomic matchingMessageReceived{false}; - auto slot = this->s_proxyConnection->addMatch(matchRule, [&](sdbus::Message& msg) + auto slot = this->s_proxyConnection->addMatch(matchRule, [&](sdbus::Message msg) { if(msg.getPath() == OBJECT_PATH) matchingMessageReceived = true; @@ -93,12 +93,12 @@ TYPED_TEST(AConnection, CanInstallMatchRuleAsynchronously) std::atomic matchingMessageReceived{false}; std::atomic matchRuleInstalled{false}; auto slot = this->s_proxyConnection->addMatchAsync( matchRule - , [&](sdbus::Message& msg) + , [&](sdbus::Message msg) { if(msg.getPath() == OBJECT_PATH) matchingMessageReceived = true; } - , [&](sdbus::Message& /*msg*/) + , [&](sdbus::Message /*msg*/) { matchRuleInstalled = true; } ); @@ -114,7 +114,7 @@ TYPED_TEST(AConnection, WillUnsubscribeMatchRuleWhenClientDestroysTheAssociatedS { auto matchRule = "sender='" + BUS_NAME + "',path='" + OBJECT_PATH + "'"; std::atomic matchingMessageReceived{false}; - auto slot = this->s_proxyConnection->addMatch(matchRule, [&](sdbus::Message& msg) + auto slot = this->s_proxyConnection->addMatch(matchRule, [&](sdbus::Message msg) { if(msg.getPath() == OBJECT_PATH) matchingMessageReceived = true; @@ -132,7 +132,7 @@ TYPED_TEST(AConnection, CanAddFloatingMatchRule) std::atomic matchingMessageReceived{false}; auto con = sdbus::createSystemBusConnection(); con->enterEventLoopAsync(); - auto callback = [&](sdbus::Message& msg) + auto callback = [&](sdbus::Message msg) { if(msg.getPath() == OBJECT_PATH) matchingMessageReceived = true; @@ -153,7 +153,7 @@ TYPED_TEST(AConnection, WillNotPassToMatchCallbackMessagesThatDoNotMatchTheRule) { auto matchRule = "type='signal',interface='" + INTERFACE_NAME + "',member='simpleSignal'"; std::atomic numberOfMatchingMessages{}; - auto slot = this->s_proxyConnection->addMatch(matchRule, [&](sdbus::Message& msg) + auto slot = this->s_proxyConnection->addMatch(matchRule, [&](sdbus::Message msg) { if(msg.getMemberName() == "simpleSignal") numberOfMatchingMessages++;