From ab39aff3238bba5cff99306e284c2d4a32cd04d1 Mon Sep 17 00:00:00 2001 From: Philip Rauwolf Date: Thu, 26 Sep 2013 11:22:02 +0200 Subject: Prevented crash on unanticipated name changes DBusServiceRegistry now can cope with reversed order of NameOwnerChanged signals. Also, as a side effect, now it copes well with name transfers. Tests for both were added, though the test for the first issue requires one private member of the ServiceRegistry to be public and therefore is commented. On the road one small performance improvement and some formatting issues were tackled. --- src/CommonAPI/DBus/DBusAddressTranslator.cpp | 2 +- src/CommonAPI/DBus/DBusConnection.cpp | 1 - src/CommonAPI/DBus/DBusConnection.h | 5 +- src/CommonAPI/DBus/DBusServiceRegistry.cpp | 59 ++++---- src/CommonAPI/DBus/DBusServiceRegistry.h | 29 +++- src/test/DBusCommunicationTest.cpp | 193 +++++++++++++++++++++++---- src/test/DBusServiceRegistryTest.cpp | 43 ++++++ 7 files changed, 268 insertions(+), 64 deletions(-) diff --git a/src/CommonAPI/DBus/DBusAddressTranslator.cpp b/src/CommonAPI/DBus/DBusAddressTranslator.cpp index 5a81199..8d89330 100644 --- a/src/CommonAPI/DBus/DBusAddressTranslator.cpp +++ b/src/CommonAPI/DBus/DBusAddressTranslator.cpp @@ -67,7 +67,7 @@ void DBusAddressTranslator::searchForDBusAddress(const std::string& commonApiAdd const auto& foundAddressMapping = commonApiAddressDetails_.find(commonApiAddress); - if(foundAddressMapping != commonApiAddressDetails_.end()) { + if (foundAddressMapping != commonApiAddressDetails_.end()) { connectionName = std::get<0>(std::get<0>(foundAddressMapping->second)); objectPath = std::get<1>(std::get<0>(foundAddressMapping->second)); interfaceName = std::get<2>(std::get<0>(foundAddressMapping->second)); diff --git a/src/CommonAPI/DBus/DBusConnection.cpp b/src/CommonAPI/DBus/DBusConnection.cpp index 1a88c60..0639305 100644 --- a/src/CommonAPI/DBus/DBusConnection.cpp +++ b/src/CommonAPI/DBus/DBusConnection.cpp @@ -1184,4 +1184,3 @@ void notifyDBusSignalHandlers(DBusSignalHandlersTable& dbusSignalHandlerstable, } // namespace DBus } // namespace CommonAPI - diff --git a/src/CommonAPI/DBus/DBusConnection.h b/src/CommonAPI/DBus/DBusConnection.h index 07e30f7..b86fd1e 100644 --- a/src/CommonAPI/DBus/DBusConnection.h +++ b/src/CommonAPI/DBus/DBusConnection.h @@ -89,8 +89,8 @@ class DBusConnection: public DBusProxyConnection, public std::enable_shared_from int timeoutMilliseconds = kDefaultSendTimeoutMs) const; DBusMessage sendDBusMessageWithReplyAndBlock(const DBusMessage& dbusMessage, - DBusError& dbusError, - int timeoutMilliseconds = kDefaultSendTimeoutMs) const; + DBusError& dbusError, + int timeoutMilliseconds = kDefaultSendTimeoutMs) const; virtual bool addObjectManagerSignalMemberHandler(const std::string& dbusBusName, DBusSignalHandler* dbusSignalHandler); @@ -203,7 +203,6 @@ class DBusConnection: public DBusProxyConnection, public std::enable_shared_from DBusConnectionStatusEvent dbusConnectionStatusEvent_; - DBusSignalMatchRulesMap dbusSignalMatchRulesMap_; DBusSignalHandlerTable dbusSignalHandlerTable_; diff --git a/src/CommonAPI/DBus/DBusServiceRegistry.cpp b/src/CommonAPI/DBus/DBusServiceRegistry.cpp index 91eaba1..98c5896 100644 --- a/src/CommonAPI/DBus/DBusServiceRegistry.cpp +++ b/src/CommonAPI/DBus/DBusServiceRegistry.cpp @@ -897,6 +897,18 @@ SubscriptionStatus DBusServiceRegistry::onDBusDaemonProxyStatusEvent(const Avail return SubscriptionStatus::RETAIN; } +void DBusServiceRegistry::checkDBusServiceWasAvailable(const std::string& dbusServiceName, + const std::string& dbusServiceUniqueName) { + + auto dbusUniqueNameIterator = dbusUniqueNamesMap_.find(dbusServiceUniqueName); + const bool isDBusUniqueNameFound = (dbusUniqueNameIterator != dbusUniqueNamesMap_.end()); + + if (isDBusUniqueNameFound) { + auto& dbusServiceListenersRecord = dbusServiceListenersMap[dbusServiceName]; + onDBusServiceNotAvailable(dbusServiceListenersRecord); + } +} + SubscriptionStatus DBusServiceRegistry::onDBusDaemonProxyNameOwnerChangedEvent(const std::string& affectedName, const std::string& oldOwner, const std::string& newOwner) { @@ -910,15 +922,7 @@ SubscriptionStatus DBusServiceRegistry::onDBusDaemonProxyNameOwnerChangedEvent(c std::lock_guard dbusServicesLock(dbusServicesMutex_); if (isDBusServiceNameLost) { - auto dbusUniqueNameIterator = dbusUniqueNamesMap_.find(dbusServiceUniqueName); - const bool isDBusUniqueNameFound = (dbusUniqueNameIterator != dbusUniqueNamesMap_.end()); - - if (isDBusUniqueNameFound) { - auto& dbusServiceListenersRecord = dbusServiceListenersMap[affectedName]; - //assert(dbusServiceListenersRecord.uniqueBusNameState == DBusRecordState::RESOLVED); - //assert(!dbusServiceListenersRecord.uniqueBusName.empty()); - onDBusServiceNotAvailable(dbusServiceListenersRecord); - } + checkDBusServiceWasAvailable(affectedName, dbusServiceUniqueName); } else { onDBusServiceAvailable(affectedName, dbusServiceUniqueName); } @@ -934,9 +938,12 @@ void DBusServiceRegistry::onDBusServiceAvailable(const std::string& dbusServiceN auto& dbusServiceListenersRecord = dbusServiceListenersMap[dbusServiceName]; const bool isDBusServiceNameObserved = !dbusServiceListenersRecord.dbusObjectPathListenersMap.empty(); - if (dbusServiceListenersRecord.uniqueBusNameState == DBusRecordState::RESOLVED) { - assert(dbusServiceListenersRecord.uniqueBusName == dbusServiceUniqueName); - return; + if (dbusServiceListenersRecord.uniqueBusNameState == DBusRecordState::RESOLVED + && dbusServiceListenersRecord.uniqueBusName != dbusServiceUniqueName) { + //A new unique connection name claims an already claimed name + //-> release of old name and claim of new name arrive in reverted order. + //Therefore: Release of old and proceed with claiming of new owner. + checkDBusServiceWasAvailable(dbusServiceName, dbusServiceListenersRecord.uniqueBusName); } dbusServiceListenersRecord.uniqueBusNameState = DBusRecordState::RESOLVED; @@ -972,7 +979,7 @@ void DBusServiceRegistry::onDBusServiceAvailable(const std::string& dbusServiceN void DBusServiceRegistry::onDBusServiceNotAvailable(DBusServiceListenersRecord& dbusServiceListenersRecord) { const std::unordered_set dbusInterfaceNamesCache; - auto dbusUniqueNameRecordIterator = dbusUniqueNamesMap_.find(dbusServiceListenersRecord.uniqueBusName); + const DBusUniqueNamesMapIterator dbusUniqueNameRecordIterator = dbusUniqueNamesMap_.find(dbusServiceListenersRecord.uniqueBusName); // fulfill all open promises on object path resolution if(dbusUniqueNameRecordIterator != dbusUniqueNamesMap_.end()) { @@ -993,9 +1000,9 @@ void DBusServiceRegistry::onDBusServiceNotAvailable(DBusServiceListenersRecord& } catch (std::future_error& e) { } } - } - removeUniqueName(dbusServiceListenersRecord.uniqueBusName); + removeUniqueName(dbusUniqueNameRecordIterator); + } dbusServiceListenersRecord.uniqueBusName.clear(); dbusServiceListenersRecord.uniqueBusNameState = DBusRecordState::NOT_AVAILABLE; @@ -1102,7 +1109,7 @@ void DBusServiceRegistry::notifyDBusObjectPathChanged(DBusInterfaceNameListeners void DBusServiceRegistry::notifyDBusInterfaceNameListeners(DBusInterfaceNameListenersRecord& dbusInterfaceNameListenersRecord, const bool& isDBusInterfaceNameAvailable) { - // FIXME maybe store simple boolean into the DBusInterfaceNameListenersRecord (only 2 states are allowed) + const AvailabilityStatus availabilityStatus = (isDBusInterfaceNameAvailable ? AvailabilityStatus::AVAILABLE : AvailabilityStatus::NOT_AVAILABLE); const DBusRecordState notifyState = (isDBusInterfaceNameAvailable ? @@ -1126,22 +1133,14 @@ void DBusServiceRegistry::notifyDBusInterfaceNameListeners(DBusInterfaceNameList } } -void DBusServiceRegistry::removeUniqueName(std::string& dbusUniqueName) { - if(dbusUniqueName.empty()) { - return; +void DBusServiceRegistry::removeUniqueName(const DBusUniqueNamesMapIterator& dbusUniqueNamesIterator) { + for (auto dbusServiceNamesIterator = dbusUniqueNamesIterator->second.ownedBusNames.begin(); + dbusServiceNamesIterator != dbusUniqueNamesIterator->second.ownedBusNames.end(); + dbusServiceNamesIterator++) { + dbusServiceNameMap_.erase(*dbusServiceNamesIterator); } - auto dbusUniqueNamesIterator = dbusUniqueNamesMap_.find(dbusUniqueName); - - if(dbusUniqueNamesIterator != dbusUniqueNamesMap_.end()) { - for (auto dbusServiceNamesIterator = dbusUniqueNamesIterator->second.ownedBusNames.begin(); - dbusServiceNamesIterator != dbusUniqueNamesIterator->second.ownedBusNames.end(); - dbusServiceNamesIterator++) { - dbusServiceNameMap_.erase(*dbusServiceNamesIterator); - } - - dbusUniqueNamesMap_.erase(dbusUniqueNamesIterator); - } + dbusUniqueNamesMap_.erase(dbusUniqueNamesIterator); } DBusServiceRegistry::DBusUniqueNameRecord* DBusServiceRegistry::insertServiceNameMapping(const std::string& dbusUniqueName, diff --git a/src/CommonAPI/DBus/DBusServiceRegistry.h b/src/CommonAPI/DBus/DBusServiceRegistry.h index c433d82..49480b1 100644 --- a/src/CommonAPI/DBus/DBusServiceRegistry.h +++ b/src/CommonAPI/DBus/DBusServiceRegistry.h @@ -91,7 +91,9 @@ class DBusServiceRegistry: public std::enable_shared_from_this getAvailableServiceInstances(const std::string& interfaceName, @@ -213,6 +215,8 @@ class DBusServiceRegistry: public std::enable_shared_from_this dbusUniqueNamesMap_; + typedef std::unordered_map::iterator DBusUniqueNamesMapIterator; + // mapping service names (well-known names) to service instances std::unordered_map dbusServiceNameMap_; @@ -248,12 +252,23 @@ class DBusServiceRegistry: public std::enable_shared_from_this #include +#include + #include #define COMMONAPI_INTERNAL_COMPILATION @@ -35,29 +37,6 @@ #include "commonapi/tests/TestInterfaceDBusProxy.h" -namespace myExtensions { - -template -class AttributeTestExtension: public CommonAPI::AttributeExtension<_AttributeType> { - typedef CommonAPI::AttributeExtension<_AttributeType> __baseClass_t; - -public: - typedef typename _AttributeType::ValueType ValueType; - typedef typename _AttributeType::AttributeAsyncCallback AttributeAsyncCallback; - - AttributeTestExtension(_AttributeType& baseAttribute) : - CommonAPI::AttributeExtension<_AttributeType>(baseAttribute) {} - - ~AttributeTestExtension() {} - - bool testExtensionMethod() const { - return true; - } -}; - -} // namespace myExtensions - - class DBusCommunicationTest: public ::testing::Test { protected: virtual void SetUp() { @@ -325,6 +304,174 @@ TEST_F(DBusCommunicationTest, RemoteMethodCallHeavyLoad) { //} + +class DBusLowLevelCommunicationTest: public ::testing::Test { + protected: + virtual void SetUp() { + runtime_ = CommonAPI::Runtime::load(); + ASSERT_TRUE((bool)runtime_); + CommonAPI::DBus::DBusRuntime* dbusRuntime = dynamic_cast(&(*runtime_)); + ASSERT_TRUE(dbusRuntime != NULL); + + proxyFactory_ = runtime_->createFactory(); + ASSERT_TRUE((bool)proxyFactory_); + + dummy = std::shared_ptr(NULL); + } + + virtual void TearDown() { + usleep(30000); + } + + std::shared_ptr createDBusStubAdapter(std::shared_ptr dbusConnection, + const std::string& commonApiAddress) { + std::string interfaceName; + std::string connectionName; + std::string objectPath; + CommonAPI::DBus::DBusAddressTranslator::getInstance().searchForDBusAddress(commonApiAddress, interfaceName, connectionName, objectPath); + + std::shared_ptr dbusStubAdapter; + std::shared_ptr stub = std::make_shared(); + + dbusStubAdapter = std::make_shared(dummy, commonApiAddress, interfaceName, connectionName, objectPath, dbusConnection, stub); + dbusStubAdapter->init(); + + CommonAPI::DBus::DBusObjectManagerStub& rootDBusObjectManagerStub = dbusConnection->getDBusObjectManager()->getRootDBusObjectManagerStub(); + + const auto dbusObjectManager = dbusConnection->getDBusObjectManager(); + const bool isDBusObjectRegistrationSuccessful = dbusObjectManager->registerDBusStubAdapter(dbusStubAdapter.get()); + + const bool isServiceExportSuccessful = rootDBusObjectManagerStub.exportDBusStubAdapter(dbusStubAdapter.get()); + + return dbusStubAdapter; + } + + std::shared_ptr runtime_; + std::shared_ptr proxyFactory_; + + std::shared_ptr dummy; + + static const std::string lowLevelAddress_; + static const std::string lowLevelConnectionName_; + static const std::string lowLevelAddress2_; + static const std::string lowLevelAddress3_; + static const std::string lowLevelAddress4_; +}; + +const std::string DBusLowLevelCommunicationTest::lowLevelAddress_ = "local:CommonAPI.DBus.tests.DBusProxyTestInterface:CommonAPI.DBus.tests.DBusProxyLowLevelService"; +const std::string DBusLowLevelCommunicationTest::lowLevelConnectionName_ = "CommonAPI.DBus.tests.DBusProxyLowLevelService"; +const std::string DBusLowLevelCommunicationTest::lowLevelAddress2_ = "local:CommonAPI.DBus.tests.DBusProxyTestInterface:CommonAPI.DBus.tests.DBusProxyLowLevelService2"; +const std::string DBusLowLevelCommunicationTest::lowLevelAddress3_ = "local:CommonAPI.DBus.tests.DBusProxyTestInterface:CommonAPI.DBus.tests.DBusProxyLowLevelService3"; +const std::string DBusLowLevelCommunicationTest::lowLevelAddress4_ = "local:CommonAPI.DBus.tests.DBusProxyTestInterface:CommonAPI.DBus.tests.DBusProxyLowLevelService4"; + +::DBusHandlerResult onLibdbusObjectPathMessageThunk(::DBusConnection* libdbusConnection, + ::DBusMessage* libdbusMessage, + void* userData) { + return ::DBusHandlerResult::DBUS_HANDLER_RESULT_HANDLED; +} + +DBusObjectPathVTable libdbusObjectPathVTable = { + NULL, + &onLibdbusObjectPathMessageThunk +}; + + +TEST_F(DBusLowLevelCommunicationTest, AgressiveNameClaimingOfServicesIsHandledCorrectly) { + std::shared_ptr connection1 = CommonAPI::DBus::DBusConnection::getSessionBus(); + std::shared_ptr connection2 = CommonAPI::DBus::DBusConnection::getSessionBus(); + + auto defaultTestProxy = proxyFactory_->buildProxy(lowLevelAddress_); + ASSERT_TRUE((bool)defaultTestProxy); + + uint32_t counter = 0; + CommonAPI::AvailabilityStatus status; + + CommonAPI::ProxyStatusEvent& proxyStatusEvent = defaultTestProxy->getProxyStatusEvent(); + proxyStatusEvent.subscribe([&counter, &status](const CommonAPI::AvailabilityStatus& stat) { + ++counter; + status = stat; + }); + + sleep(1); + + EXPECT_EQ(1, counter); + EXPECT_EQ(CommonAPI::AvailabilityStatus::NOT_AVAILABLE, status); + + //Set up low level connections + ::DBusConnection* libdbusConnection1 = dbus_bus_get_private(DBUS_BUS_SESSION, NULL); + ::DBusConnection* libdbusConnection2 = dbus_bus_get_private(DBUS_BUS_SESSION, NULL); + + ASSERT_TRUE(libdbusConnection1); + ASSERT_TRUE(libdbusConnection2); + + dbus_connection_set_exit_on_disconnect(libdbusConnection1, false); + dbus_connection_set_exit_on_disconnect(libdbusConnection2, false); + + bool endDispatch = false; + std::promise ended; + std::future hasEnded = ended.get_future(); + + std::thread([&]() { + dbus_bool_t libdbusSuccess = true; + while (!endDispatch && libdbusSuccess) { + libdbusSuccess = dbus_connection_read_write_dispatch(libdbusConnection1, 10); + libdbusSuccess &= dbus_connection_read_write_dispatch(libdbusConnection2, 10); + } + ended.set_value(true); + }).detach(); + + //Test first connect + std::shared_ptr dbusConnection1 = std::make_shared(libdbusConnection1); + ASSERT_TRUE(dbusConnection1->isConnected()); + std::shared_ptr adapter1 = createDBusStubAdapter(dbusConnection1, lowLevelAddress_); + + int libdbusStatus = dbus_bus_request_name(libdbusConnection1, + lowLevelConnectionName_.c_str(), + DBUS_NAME_FLAG_ALLOW_REPLACEMENT | DBUS_NAME_FLAG_REPLACE_EXISTING, + NULL); + + dbus_connection_try_register_object_path(libdbusConnection1, + "/", + &libdbusObjectPathVTable, + NULL, + NULL); + + sleep(1); + + EXPECT_EQ(DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER, libdbusStatus); + EXPECT_EQ(2, counter); + EXPECT_EQ(CommonAPI::AvailabilityStatus::AVAILABLE, status); + + //Test second connect + std::shared_ptr dbusConnection2 = std::make_shared(libdbusConnection2); + ASSERT_TRUE(dbusConnection2->isConnected()); + std::shared_ptr adapter2 = createDBusStubAdapter(dbusConnection2, lowLevelAddress_); + + libdbusStatus = dbus_bus_request_name(libdbusConnection2, + lowLevelConnectionName_.c_str(), + DBUS_NAME_FLAG_ALLOW_REPLACEMENT | DBUS_NAME_FLAG_REPLACE_EXISTING, + NULL); + + dbus_connection_try_register_object_path(libdbusConnection2, + "/", + &libdbusObjectPathVTable, + NULL, + NULL); + + sleep(1); + + EXPECT_EQ(DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER, libdbusStatus); + + //4 Because a short phase of non-availability will be inbetween + EXPECT_EQ(4, counter); + EXPECT_EQ(CommonAPI::AvailabilityStatus::AVAILABLE, status); + + //Close connections + endDispatch = true; + ASSERT_TRUE(hasEnded.get()); +} + + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/src/test/DBusServiceRegistryTest.cpp b/src/test/DBusServiceRegistryTest.cpp index a429291..170f8f6 100644 --- a/src/test/DBusServiceRegistryTest.cpp +++ b/src/test/DBusServiceRegistryTest.cpp @@ -363,6 +363,49 @@ TEST_F(DBusServiceRegistryTest, DBusAddressTranslatorPredefinedWorks) { } } + +CommonAPI::DBus::DBusMessage getNewFakeNameOwnerChangedMessage() { + static const char* objectPath = "/"; + static const char* interfaceName = "org.freedesktop.DBus"; + static const char* signalName = "NameOwnerChanged"; + return CommonAPI::DBus::DBusMessage::createSignal(objectPath, interfaceName, signalName, "sss"); +} + + +//XXX This test requires CommonAPI::DBus::DBusServiceRegistry::onDBusDaemonProxyNameOwnerChangedEvent to be public! + +//TEST_F(DBusServiceRegistryTest, RevertedNameOwnerChangedEventsWork) { +// std::shared_ptr registryConnection = CommonAPI::DBus::DBusConnection::getSessionBus(); +// std::shared_ptr registry = std::make_shared(registryConnection); +// +// registry->init(); +// +// CommonAPI::DBus::DBusMessage fakeNameOwnerChangedSignal1 = getNewFakeNameOwnerChangedMessage(); +// CommonAPI::DBus::DBusMessage fakeNameOwnerChangedSignal2 = getNewFakeNameOwnerChangedMessage(); +// CommonAPI::DBus::DBusMessage fakeNameOwnerChangedSignal3 = getNewFakeNameOwnerChangedMessage(); +// +// CommonAPI::DBus::DBusOutputStream outStream1(fakeNameOwnerChangedSignal1); +// CommonAPI::DBus::DBusOutputStream outStream2(fakeNameOwnerChangedSignal2); +// CommonAPI::DBus::DBusOutputStream outStream3(fakeNameOwnerChangedSignal3); +// +// std::string serviceName = "my.fake.service"; +// std::string emptyOwner = ""; +// std::string newOwner1 = ":1:505"; +// std::string newOwner2 = ":1:506"; +// +// outStream1 << serviceName << emptyOwner << newOwner1; +// outStream1.flush(); +// outStream2 << serviceName << emptyOwner << newOwner2; +// outStream2.flush(); +// outStream3 << serviceName << newOwner1 << emptyOwner; +// outStream3.flush(); +// +// registry->onDBusDaemonProxyNameOwnerChangedEvent(serviceName, emptyOwner, newOwner1); +// registry->onDBusDaemonProxyNameOwnerChangedEvent(serviceName, emptyOwner, newOwner2); +// registry->onDBusDaemonProxyNameOwnerChangedEvent(serviceName, newOwner1, emptyOwner); +//} + + TEST_F(DBusServiceRegistryTest, PredefinedInstances) { // auto stubDBusConnection = CommonAPI::DBus::DBusConnection::getSessionBus(); // -- cgit v1.2.1