feedc0de/sdbus-cpp
1
0
Fork 0
forked from Kistler-Group/sdbus-cpp
Code Pull Requests Activity

refactor: let callbacks take message objects by value (#367)

Browse Source 
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.
This commit is contained in:
Stanislav Angelovič
2023-10-24 21:56:47 +02:00
committed by Stanislav Angelovic
parent 21dd77ba6c
commit 071e38c9de
10 changed files with 37 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
ChangeLog
View File

@ -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)

6
docs/using-sdbus-c++.md
View File

@ -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
{

6
include/sdbus-c++/ConvenienceApiClasses.inl
View File

@ -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.

2
include/sdbus-c++/IConnection.h
View File

@ -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

8
include/sdbus-c++/TypeTraits.h
View File

@ -62,10 +62,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 signal_handler = std::function<void(Signal& signal)>;
using message_handler = std::function<void(Message& msg)>;
using property_set_callback = std::function<void(PropertySetCall& msg)>;
using async_reply_handler = std::function<void(MethodReply reply, const Error* error)>;
using signal_handler = std::function<void(Signal signal)>;
using message_handler = std::function<void(Message 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++

4
src/Connection.cpp
View File

@ -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;
}

4
src/Object.cpp
View File

@ -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;
}

10
src/Proxy.cpp
View File

@ -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;
}

2
src/Proxy.h
View File

@ -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

12
tests/integrationtests/DBusGeneralTests.cpp
View File

@ -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++;