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

@@ -257,14 +257,16 @@ v1.6.0
 
 v2.0.0
 - 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,
+  - 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
+    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 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.
+  - Signatures of callbacks `async_reply_handler`, `signal_handler`, `message_handler` and `property_set_callback` were modified to take input message objects
-   - `PollData::timeout_usec` was renamed to `PollData::timeout` and its type has been changed to `std::chrono::microseconds`. This member now holds directly
+    by value, as opposed to non-const ref. Callee assumes ownership of the message. This API is more idiomatic, cleaner and safer.
-     what before had to be obtained through `PollData::getAbsoluteTimeout()` call.
+  - 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::getRelativeTimeout()` return type was changed to `std::chrono::microseconds`.
+  - `PollData::timeout_usec` was renamed to `PollData::timeout` and its type has been changed to `std::chrono::microseconds`. This member now holds directly
-   - `IConnection::processPendingRequest()` was renamed to `IConnection::processPendingEvent()`.
+    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)

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 <iostream>
 #include <unistd.h>
 
-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
         {

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.

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

include/sdbus-c++/TypeTraits.h

@@ -63,10 +63,10 @@ namespace sdbus {
 
     // Callbacks from sdbus-c++
     using method_callback = std::function<void(MethodCall msg)>;
-    using async_reply_handler = std::function<void(MethodReply& reply, const Error* error)>;
+    using async_reply_handler = std::function<void(MethodReply reply, const Error* error)>;
-    using signal_handler = std::function<void(Signal& signal)>;
+    using signal_handler = std::function<void(Signal signal)>;
-    using message_handler = std::function<void(Message& msg)>;
+    using message_handler = std::function<void(Message msg)>;
-    using property_set_callback = std::function<void(PropertySetCall& msg)>;
+    using property_set_callback = std::function<void(PropertySetCall msg)>;
     using property_get_callback = std::function<void(PropertyGetReply& reply)>;
 
     // Type-erased RAII-style handle to callbacks/subscriptions registered to sdbus-c++

src/Connection.cpp

@@ -811,7 +811,7 @@ int Connection::sdbus_match_callback(sd_bus_message *sdbusMessage, void *userDat
 {
     auto* matchInfo = static_cast<MatchInfo*>(userData);
     auto message = Message::Factory::create<PlainMessage>(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<MatchInfo*>(userData);
     auto message = Message::Factory::create<PlainMessage>(sdbusMessage, &matchInfo->connection.getSdBusInterface());
-    auto ok = invokeHandlerAndCatchErrors([&](){ matchInfo->installCallback(message); }, retError);
+    auto ok = invokeHandlerAndCatchErrors([&](){ matchInfo->installCallback(std::move(message)); }, retError);
     return ok ? 0 : -1;
 }
 

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<PropertySetCall>(sdbusValue, &object.connection_.getSdBusInterface());
 
-    auto ok = invokeHandlerAndCatchErrors([&](){ callback(value); }, retError);
+    auto ok = invokeHandlerAndCatchErrors([&](){ callback(std::move(value)); }, retError);
 
     return ok ? 1 : -1;
 }

src/Proxy.cpp

@@ -119,10 +119,10 @@ std::future<MethodReply> Proxy::callMethod(const MethodCall& message, uint64_t t
     auto promise = std::make_shared<std::promise<MethodReply>>();
     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<Signal>(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;
 }

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

tests/integrationtests/DBusGeneralTests.cpp

@@ -76,7 +76,7 @@ TYPED_TEST(AConnection, WillCallCallbackHandlerForIncomingMessageMatchingMatchRu
 {
     auto matchRule = "sender='" + BUS_NAME + "',path='" + OBJECT_PATH + "'";
     std::atomic<bool> 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<bool> matchingMessageReceived{false};
     std::atomic<bool> 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<bool> 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<bool> 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<size_t> 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++;