From 65caef7ee6bbb7d99e700f981b5e2f34a9a5a38e Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 13 May 2013 08:06:01 -0700 Subject: D-Bus: fix syncevo-dbus-server<->syncevo-dbus-helper communication when using GIO D-Bus D-Bus objects in a process exist independently from a specific D-Bus connection. They are only identified by their path. When syncevo-dbus-server forked multiple syncevo-dbus-helper instances, it indirectly (in ForkExec) created multiple callback objects for the childs "watch" functionality (a pending method call that the parent never replies to). These method calls were associated with the a random (oldest?) session instead of the current one. This causes problems once an older session terminates and the callback object destructs, because then the wrong children get a failure response, which they treat as "connection lost = parent has terminated". Found while trying to fix shutdown race conditions. The solution is to generate a unique counter for each child and communicate that counter to the child via a new env variable. --- src/dbus/server/session-helper.cpp | 2 +- src/dbus/server/session.cpp | 9 ++++---- src/syncevo/ForkExec.cpp | 42 +++++++++++++++++++++++++++++++------- src/syncevo/ForkExec.h | 8 ++++++++ 4 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/dbus/server/session-helper.cpp b/src/dbus/server/session-helper.cpp index 0df0004b..0ae993d9 100644 --- a/src/dbus/server/session-helper.cpp +++ b/src/dbus/server/session-helper.cpp @@ -107,7 +107,7 @@ SessionHelper::SessionHelper(GMainLoop *loop, const boost::shared_ptr &forkexec, const boost::shared_ptr &parentLogger) : GDBusCXX::DBusObjectHelper(conn, - SessionCommon::HELPER_PATH, + std::string(SessionCommon::HELPER_PATH) + "/" + forkexec->getInstance(), SessionCommon::HELPER_IFACE, GDBusCXX::DBusObjectHelper::Callback_t(), // we don't care about a callback per message true), // direct connection, close it when done diff --git a/src/dbus/server/session.cpp b/src/dbus/server/session.cpp index acae499c..134c8e17 100644 --- a/src/dbus/server/session.cpp +++ b/src/dbus/server/session.cpp @@ -43,9 +43,9 @@ SE_BEGIN_CXX class SessionProxy : public GDBusCXX::DBusRemoteObject { public: - SessionProxy(const GDBusCXX::DBusConnectionPtr &conn) : + SessionProxy(const GDBusCXX::DBusConnectionPtr &conn, const std::string &instance) : GDBusCXX::DBusRemoteObject(conn.get(), - SessionCommon::HELPER_PATH, + std::string(SessionCommon::HELPER_PATH) + "/" + instance, SessionCommon::HELPER_IFACE, SessionCommon::HELPER_DESTINATION, true), // This is a one-to-one connection. Close it. @@ -874,8 +874,9 @@ void Session::onConnect(const GDBusCXX::DBusConnectionPtr &conn) throw () { PushLogger guard(m_me); try { - SE_LOG_DEBUG(NULL, "helper has connected"); - m_helper.reset(new SessionProxy(conn)); + std::string instance = m_forkExecParent->getInstance(); + SE_LOG_DEBUG(NULL, "helper %s has connected", instance.c_str()); + m_helper.reset(new SessionProxy(conn, instance)); // Activate signal watch on helper signals. m_helper->m_syncProgress.activate(boost::bind(&Session::syncProgress, this, _1, _2, _3, _4)); diff --git a/src/syncevo/ForkExec.cpp b/src/syncevo/ForkExec.cpp index 46cd1d67..14ad4a6b 100644 --- a/src/syncevo/ForkExec.cpp +++ b/src/syncevo/ForkExec.cpp @@ -19,15 +19,18 @@ #include "ForkExec.h" #include +#include #if defined(HAVE_GLIB) #include +#include #include "test.h" SE_BEGIN_CXX static const std::string ForkExecEnvVar("SYNCEVOLUTION_FORK_EXEC="); +static const std::string ForkExecInstanceEnvVar("SYNCEVOLUTION_FORK_EXEC_INSTANCE="); #ifndef GDBUS_CXX_HAVE_DISCONNECT // internal D-Bus API: only used to monitor parent by having one method call pending @@ -44,18 +47,34 @@ static const std::string FORKEXEC_PARENT_DESTINATION = "direct.peer"; // doesn't class ForkExecParentDBusAPI : public GDBusCXX::DBusObjectHelper { public: - ForkExecParentDBusAPI(const GDBusCXX::DBusConnectionPtr &conn) : + /** + * @param instance a unique string to distinguish multiple different ForkExecParent + * instances; necessary because otherwise GIO GDBus may route messages from + * one connection to older instances on other connections + */ + ForkExecParentDBusAPI(const GDBusCXX::DBusConnectionPtr &conn, const std::string &instance) : GDBusCXX::DBusObjectHelper(conn, - FORKEXEC_PARENT_PATH, + FORKEXEC_PARENT_PATH + "/" + instance, FORKEXEC_PARENT_IFACE) { add(this, &ForkExecParentDBusAPI::watch, "Watch"); activate(); } + ~ForkExecParentDBusAPI() + { + SE_LOG_DEBUG(NULL, "ForkExecParentDBusAPI %s: destroying with %ld active watches", + getPath(), + (long)m_watches.size()); + } + + bool hasWatches() const { return !m_watches.empty(); } + private: void watch(const boost::shared_ptr< GDBusCXX::Result0> &result) { + SE_LOG_DEBUG(NULL, "ForkExecParentDBusAPI %s: received 'Watch' method call from child", + getPath()); m_watches.push_back(result); } std::list< boost::shared_ptr< GDBusCXX::Result0> > m_watches; @@ -66,6 +85,9 @@ ForkExec::ForkExec() { } +static Mutex ForkExecMutex; +static unsigned int ForkExecCount; + ForkExecParent::ForkExecParent(const std::string &helper) : m_helper(helper), m_childPid(0), @@ -81,6 +103,9 @@ ForkExecParent::ForkExecParent(const std::string &helper) : m_errID(0), m_watchChild(NULL) { + Mutex::Guard guard = ForkExecMutex.lock(); + ForkExecCount++; + m_instance = StringPrintf("forkexec%u", ForkExecCount); } boost::shared_ptr ForkExecParent::create(const std::string &helper) @@ -182,13 +207,15 @@ void ForkExecParent::start() for (char **env = environ; *env; env++) { - if (!boost::starts_with(*env, ForkExecEnvVar)) { + if (!boost::starts_with(*env, ForkExecEnvVar) && + !boost::starts_with(*env, ForkExecInstanceEnvVar)) { m_envStrings.push_back(*env); } } // pass D-Bus address via env variable m_envStrings.push_back(ForkExecEnvVar + m_server->getAddress()); + m_envStrings.push_back(ForkExecInstanceEnvVar + getInstance()); m_env.reset(AllocStringArray(m_envStrings)); SE_LOG_DEBUG(NULL, "ForkExecParent: running %s with D-Bus address %s", @@ -375,7 +402,7 @@ void ForkExecParent::newClientConnection(GDBusCXX::DBusConnectionPtr &conn) thro m_helper.c_str()); m_hasConnected = true; #ifndef GDBUS_CXX_HAVE_DISCONNECT - m_api.reset(new ForkExecParentDBusAPI(conn)); + m_api.reset(new ForkExecParentDBusAPI(conn, getInstance())); #endif m_onConnect(conn); } catch (...) { @@ -442,6 +469,7 @@ void ForkExecParent::kill() ForkExecChild::ForkExecChild() : m_state(IDLE) { + m_instance = getEnv(ForkExecInstanceEnvVar.substr(0, ForkExecInstanceEnvVar.size() - 1).c_str(), ""); } boost::shared_ptr ForkExecChild::create() @@ -480,16 +508,16 @@ void ForkExecChild::connect() class Parent : public GDBusCXX::DBusRemoteObject { public: - Parent(const GDBusCXX::DBusConnectionPtr &conn) : + Parent(const GDBusCXX::DBusConnectionPtr &conn, const std::string &instance) : GDBusCXX::DBusRemoteObject(conn, - FORKEXEC_PARENT_PATH, + FORKEXEC_PARENT_PATH + "/" + instance, FORKEXEC_PARENT_IFACE, FORKEXEC_PARENT_DESTINATION), m_watch(*this, "Watch") {} GDBusCXX::DBusClientCall0 m_watch; - } parent(conn); + } parent(conn, getInstance()); parent.m_watch.start(boost::bind(&ForkExecChild::connectionLost, this)); #endif diff --git a/src/syncevo/ForkExec.h b/src/syncevo/ForkExec.h index 5de1848c..e460e59d 100644 --- a/src/syncevo/ForkExec.h +++ b/src/syncevo/ForkExec.h @@ -100,8 +100,16 @@ class ForkExec : private boost::noncopyable { typedef boost::signals2::signal OnFailure; OnFailure m_onFailure; + /** + * A unique string for the ForkExecParent/Child pair which can be used + * as D-Bus path component. + */ + std::string getInstance() const { return m_instance; } + protected: ForkExec(); + + std::string m_instance; }; /** -- cgit v1.2.3