Library Additions and Issues for Discussion

[dleeds-cpi]

@sangelovic after using sdbus-c++ for a few months, I jotted down a list wants and issues I have encountered. I would love your and the community's feedback on these. Are you aware of them? Also, I would be happy to contribute to add some of these. Are you open to pull requests?

API Improvements

• Either make createConnection() call sd_bus_open() instead of sd_bus_open_system(), or add a createDefaultBusConnection() to do it, in order to maintain backward compatibility. (Add createDefaultBusConnection() #182) DONE

• Add IProxy::getConnection() (PR: Add IProxy::getConnection() #179) DONE

• Add IConnection::registerSignalHandler(const std::string &matchRule). Manage match rule lifetime using a handle with a weak_ptr. (PR: feat: add support for match rules #264) DONE (@dleeds-cpi please double-check)

• Add ability to setDestination() for Signal (PR: Add API to set signal destination #168) DONE

• Fix ObjectManager_proxy. Currently, can only receive InterfacesAdded/Removed signals for itself, which defeats the purpose, as we often cannot create a proxy for an object that hasn't been added yet (its path is unknown). Usually, clients using the ObjectManager listen for all InterfacesAdded/Removed signals emitted for objects under a root path. Preferably, should use a 'path_namespace' match rule, not a 'path' match, so the proxy can monitor child object creation and deletion and add/remove proxies for them as needed. (PR: Issue 139 object manager API #207) DONE

• Async property get/set capability (PR: feat: add support for async property get/set on client-side #354) DONE

• Add default constructor for PendingAsyncCall. It is helpful to allow creation of an empty handle. (Enable default construction of PendingAsyncCall #180) DONE

• Enable r-value construction and assignment for generated adaptor and proxy classes (PR: refactor: support move semantics in generated adaptor and proxy classes #286) DONE

Performance

• Potential performance issue: Message::createPlainMessage() is used for every Variant and opens and closes a system bus connection. It is a bit of a hack, but you should be able to do sd_bus_new() + sd_bus_start(), instead of a full bus open. sd_bus_start() will fail due to having not set a bus address, but it will leave the bus in an OPENING state, which is enough to call sd_bus_message_new() for createPlainMessage() (PR: refactor: use pseudo D-Bus connection for plain messages #280) DONE

Bugs

• When using the Convenience API and an event loop, receiving a message with an unexpected payload results in an exception propagating directly to the event loop callback (which generally do not catch exceptions), and terminating the program. Malformed messages received asynchronously from external sources should be dropped without raising an exception. (Avoid propagating msg unpack exceptions to event loop #193) DONE

• Seeing EINVAL exception on Variant construction (from createPlainMessage()) (not visible on all environments (e.g. x86, g++ 9.x), seeing it on buildroot, systemd 245, aarch64-none-linux-gnu-g++ 9.2.1) FIXED in systemd: Compatibility with systemd 245.6 #106

• SEGV crash on Object destruction during stack unwinding (seems to be a double free/ordering issue). Worked around this by calling IObject::unregister() before destruction. (Issue 133 race in proxy destruct  #177) DONE

#0  0x00005555557965fb in sdbus::internal::Connection::addObjectVTable(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, sd_bus_vtable const*, void*)::{lambda(void*)#1}::operator()(void*) const ()
#1  0x0000555555799afd in std::_Function_handler<void (void*), sdbus::internal::Connection::addObjectVTable(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, sd_bus_vtable const*, void*)::{lambda(void*)#1}>::_M_invoke(std::_Any_data const&, void*&&) ()
#2  0x000055555579bd87 in std::function<void (void*)>::operator()(void*) const ()
#3  0x000055555579b0b8 in std::unique_ptr<void, std::function<void (void*)> >::~unique_ptr() ()
#4  0x00005555557ad0e4 in sdbus::internal::Object::InterfaceData::~InterfaceData() ()
#5  0x00005555557ad146 in std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData>::~pair() ()
#6  0x00005555557ad176 in void __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> > >::destroy<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> >(std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData>*) ()
#7  0x00005555557ac841 in void std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> > > >::destroy<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> >(std::allocator<std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> > >&, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData>*) ()
#8  0x00005555557aaf6d in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> > >::_M_destroy_node(std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> >*) ()
#9  0x00005555557a8daf in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> > >::_M_drop_node(std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> >*) ()
#10 0x00005555557a78ce in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> > >::_M_erase(std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> >*) ()
#11 0x00005555557a6878 in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> > >::~_Rb_tree() ()
#12 0x00005555557a66ea in std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, sdbus::internal::Object::InterfaceData, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, sdbus::internal::Object::InterfaceData> > >::~map() ()
#13 0x00005555557a8c3c in sdbus::internal::Object::~Object() ()
#14 0x00005555557a8c78 in sdbus::internal::Object::~Object() ()
#15 0x00005555556d6ea5 in std::default_delete<sdbus::IObject>::operator() (this=0x7fffffffdc78, __ptr=0x60d000002dc0) at /usr/include/c++/9/bits/unique_ptr.h:81
#16 0x00005555556d43d5 in std::unique_ptr<sdbus::IObject, std::default_delete<sdbus::IObject> >::~unique_ptr (this=0x7fffffffdc78, __in_chrg=<optimized out>)
    at /usr/include/c++/9/bits/unique_ptr.h:292
#17 ----------------- snip --- containing object destructor -------------------

[sangelovic]

@dleeds-cpi hi, thanks a lot for your suggestions, and sorry for late reply, I'm now a busy man :) I've quickly gone through that and it's good to see such a nice list of ideas, improvements, defects. And, of course, we are open to whatever improves the library :) So, if you'd love to contribute, don't hesitate and go ahead, and either start a discussion with us on the specific topic here in Discussions section (might be good choice to discuss beforehand if you're unsure about proper solution), or directly submit a PR. Do you already have an idea which items in the list you'd like to cover?

[dleeds-cpi]

Hi @sangelovic I understand your time limitations, as I am also subject those! I will set aside some time shortly to put up some PRs for some minor API additions described above, and work on the larger things when possible.

[dleeds-cpi]

We've worked through most of the above points! The only significant one remaining is the issue with the ObjectManager_adaptor, but I realized that it can be worked around simply by not using it.

@sangelovic what do you think about creating a new tag? 0.9.0? Also, what is the criteria for going to 1.0? Perhaps update the ObjectManager_adaptor interface, and it's there?

[sangelovic]

@dleeds-cpi Perfect, that is also thanks to your contributions! I guess we can create a new tag, 0.9.0, then proceed with fixing the object manager, then releasing 1.0. Or we may be more patient, wait for object manager fix, and go to 1.0 directly.

[sangelovic]

@dleeds-cpi I've just tagged the library with v0.9.0, before merging object manager changes.

/cc @riuriuriu

[sangelovic]

Hey @dleeds-cpi, how is life going? I've gone through the list of wants and issues of yours, and I've updated it with relevant PRs. As far as I understand all the points, I think everything is done (i.e. in master, or in release/v2.0 branch) now. I'm however not 100% sure regarding your point about IConnection::registerSignalHandler(const std::string &matchRule). Match rules are implemented (albeit only synchronous registration of those). Can you double-check? I think I've seen somewhere a proposal to implement an async registration of match rules -- was that from you?

[dleeds-cpi]

Hi, thanks for following up. I have been really excited about all the recent feature additions, and I actually use the match rule feature and sync registration is fine for my needs, although the async option might be nice just for completeness.

Now, if you want to brainstorm how to implement async calls using C++20 coroutines, that would be a topic of interest. Unfortunately, I have been doing research on this lately, and it seems that coroutine support up to this point must be coupled with a support library and a scheduler implementation, and because those are not provided by STL, it is a barrier to adoption. I have been experimenting with https://github.com/jbaldwin/libcoro, which looks like a well crafted evolution of Lewis Baker's original reference library (that is somewhat out of date now): https://github.com/lewissbaker/cppcoro. These libraries have not achieved universal adoption, so it would be difficult to justify a full integration.