Fix glib callbacks lifecycle problems
authorPiotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
Fri, 11 Apr 2014 08:58:47 +0000 (10:58 +0200)
committerJan Olszak <j.olszak@samsung.com>
Mon, 19 May 2014 11:47:15 +0000 (13:47 +0200)
[Bug/Feature]   Dbus connection callbacks was called on destroyed
                connection.
[Cause]         Closing glib connection does not remove pending events
                so they can be fired in glib loop thread later.
[Solution]      Wait for all callbacks to be deleted before connection
                destructor ends.
[Verification]  Build, install, run tests.

Change-Id: Id8e1999cf5938be64493cac503fbae1015abc02e

common/dbus/connection.cpp
common/dbus/connection.hpp
common/utils/callback-guard.cpp [new file with mode: 0644]
common/utils/callback-guard.hpp [new file with mode: 0644]
common/utils/callback-wrapper.hpp [new file with mode: 0644]
unit_tests/utils/ut-callback-guard.cpp [new file with mode: 0644]

index 609a8bd..75898f8 100644 (file)
@@ -24,6 +24,7 @@
 
 #include "dbus/connection.hpp"
 #include "dbus/exception.hpp"
+#include "utils/callback-wrapper.hpp"
 #include "log/logger.hpp"
 
 
@@ -39,12 +40,6 @@ const std::string INTROSPECT_METHOD = "Introspect";
 
 const int CALL_METHOD_TIMEOUT_MS = 1000;
 
-template<class Callback>
-void deleteCallback(gpointer data)
-{
-    delete reinterpret_cast<Callback*>(data);
-}
-
 class ScopedError {
 public:
     ScopedError() : mError(NULL) {}
@@ -160,10 +155,6 @@ DbusConnection::~DbusConnection()
     if (mNameId) {
         g_bus_unown_name(mNameId);
     }
-    //TODO should we unregister, flush, close etc?
-    //if (!g_dbus_connection_close_sync(mConnection, NULL, NULL)) {
-    //    LOGE("Could not close connection");
-    //}
     g_object_unref(mConnection);
     LOGT("Connection deleted");
 }
@@ -177,14 +168,16 @@ void DbusConnection::setName(const std::string& name,
                                            G_BUS_NAME_OWNER_FLAGS_NONE,
                                            &DbusConnection::onNameAcquired,
                                            &DbusConnection::onNameLost,
-                                           new NameCallbacks(onNameAcquired, onNameLost),
-                                           &deleteCallback<NameCallbacks>);
+                                           utils::createCallbackWrapper(
+                                               NameCallbacks(onNameAcquired, onNameLost),
+                                               mGuard.spawn()),
+                                           &utils::deleteCallbackWrapper<NameCallbacks>);
 }
 
 void DbusConnection::onNameAcquired(GDBusConnection*, const gchar* name, gpointer userData)
 {
     LOGD("Name acquired " << name);
-    const NameCallbacks& callbacks = *reinterpret_cast<const NameCallbacks*>(userData);
+    const NameCallbacks& callbacks = utils::getCallbackFromPointer<NameCallbacks>(userData);
     if (callbacks.nameAcquired) {
         callbacks.nameAcquired();
     }
@@ -193,7 +186,7 @@ void DbusConnection::onNameAcquired(GDBusConnection*, const gchar* name, gpointe
 void DbusConnection::onNameLost(GDBusConnection*, const gchar* name, gpointer userData)
 {
     LOGE("Name lost " << name);
-    const NameCallbacks& callbacks = *reinterpret_cast<const NameCallbacks*>(userData);
+    const NameCallbacks& callbacks = utils::getCallbackFromPointer<NameCallbacks>(userData);
     if (callbacks.nameLost) {
         callbacks.nameLost();
     }
@@ -230,8 +223,8 @@ void DbusConnection::signalSubscribe(const SignalCallback& callback,
                                        NULL,
                                        G_DBUS_SIGNAL_FLAGS_NONE,
                                        &DbusConnection::onSignal,
-                                       new SignalCallback(callback),
-                                       &deleteCallback<SignalCallback>);
+                                       utils::createCallbackWrapper(callback, mGuard.spawn()),
+                                       &utils::deleteCallbackWrapper<SignalCallback>);
 }
 
 void DbusConnection::onSignal(GDBusConnection*,
@@ -242,7 +235,7 @@ void DbusConnection::onSignal(GDBusConnection*,
                               GVariant* parameters,
                               gpointer userData)
 {
-    const SignalCallback& callback = *static_cast<const SignalCallback*>(userData);
+    const SignalCallback& callback = utils::getCallbackFromPointer<SignalCallback>(userData);
 
     LOGD("Signal: " << sender << "; " << object << "; " << interface << "; " << name);
 
@@ -295,8 +288,8 @@ void DbusConnection::registerObject(const std::string& objectPath,
                                       objectPath.c_str(),
                                       interfaceInfo,
                                       &vtable,
-                                      new MethodCallCallback(callback),
-                                      &deleteCallback<MethodCallCallback>,
+                                      utils::createCallbackWrapper(callback, mGuard.spawn()),
+                                      &utils::deleteCallbackWrapper<MethodCallCallback>,
                                       &error);
     g_dbus_node_info_unref(nodeInfo);
     if (error) {
@@ -315,7 +308,7 @@ void DbusConnection::onMethodCall(GDBusConnection*,
                                   GDBusMethodInvocation* invocation,
                                   gpointer userData)
 {
-    const MethodCallCallback& callback = *static_cast<const MethodCallCallback*>(userData);
+    const MethodCallCallback& callback = utils::getCallbackFromPointer<MethodCallCallback>(userData);
 
     LOGD("MethodCall; " << objectPath << "; " << interface << "; " << method);
 
index 514a87c..cbdbdf5 100644 (file)
@@ -25,6 +25,8 @@
 #ifndef COMMON_DBUS_CONNECTION_HPP
 #define COMMON_DBUS_CONNECTION_HPP
 
+#include "utils/callback-guard.hpp"
+
 #include <memory>
 #include <string>
 #include <functional>
@@ -141,6 +143,7 @@ private:
             : nameAcquired(acquired), nameLost(lost) {}
     };
 
+    utils::CallbackGuard mGuard;
     GDBusConnection* mConnection;
     guint mNameId;
 
diff --git a/common/utils/callback-guard.cpp b/common/utils/callback-guard.cpp
new file mode 100644 (file)
index 0000000..d1e7084
--- /dev/null
@@ -0,0 +1,127 @@
+/*
+ *  Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ *  Contact: Piotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License
+ */
+
+/**
+ * @file
+ * @author  Piotr Bartosiewicz (p.bartosiewi@partner.samsung.com)
+ * @brief   Callback guard
+ */
+
+#include "callback-guard.hpp"
+#include "log/logger.hpp"
+
+#include <mutex>
+#include <condition_variable>
+#include <cassert>
+
+
+namespace security_containers {
+namespace utils {
+
+// Reference counting class like shared_ptr but with the ability to wait for it.
+class CallbackGuard::SharedState {
+public:
+    void inc()
+    {
+        std::unique_lock<std::mutex> lock(mMutex);
+        ++mCounter;
+    }
+
+    void dec()
+    {
+        std::unique_lock<std::mutex> lock(mMutex);
+        --mCounter;
+        mEmptyCondition.notify_all();
+    }
+
+    long size()
+    {
+        std::unique_lock<std::mutex> lock(mMutex);
+        return mCounter;
+    }
+
+    bool wait(const unsigned int timeoutMs)
+    {
+        std::unique_lock<std::mutex> lock(mMutex);
+        return mEmptyCondition.wait_for(lock,
+                                        std::chrono::milliseconds(timeoutMs),
+                                        [this] {return mCounter == 0;});
+    }
+private:
+    std::mutex mMutex;
+    std::condition_variable mEmptyCondition;
+    long mCounter = 0;
+};
+
+namespace {
+
+template<class SharedState>
+class TrackerImpl {
+public:
+    TrackerImpl(const std::shared_ptr<SharedState>& sharedState)
+        : mSharedState(sharedState)
+    {
+        mSharedState->inc();
+    }
+
+    ~TrackerImpl()
+    {
+        mSharedState->dec();
+    }
+private:
+    std::shared_ptr<SharedState> mSharedState;
+};
+
+// Relatively big timeout in case of deadlock.
+// This timeout should never be exceeded with
+// a properly written code.
+const unsigned int TIMEOUT = 5000;
+
+} // namespace
+
+
+CallbackGuard::CallbackGuard()
+    : mSharedState(new SharedState())
+{
+}
+
+CallbackGuard::~CallbackGuard()
+{
+    if (!waitForTrackers(TIMEOUT)) {
+        LOGE("==== DETECTED INVALID CALLBACK USE ====");
+        assert(0 && "Invalid callback use");
+    }
+}
+
+CallbackGuard::Tracker CallbackGuard::spawn() const
+{
+    return Tracker(new TrackerImpl<SharedState>(mSharedState));
+}
+
+long CallbackGuard::getTrackersCount() const
+{
+    return mSharedState->size();
+}
+
+bool CallbackGuard::waitForTrackers(const unsigned int timeoutMs)
+{
+    return mSharedState->wait(timeoutMs);
+}
+
+} // namespace utils
+} // namespace security_containers
diff --git a/common/utils/callback-guard.hpp b/common/utils/callback-guard.hpp
new file mode 100644 (file)
index 0000000..3ebc2fb
--- /dev/null
@@ -0,0 +1,79 @@
+/*
+ *  Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ *  Contact: Piotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License
+ */
+
+/**
+ * @file
+ * @author  Piotr Bartosiewicz (p.bartosiewi@partner.samsung.com)
+ * @brief   Callback guard
+ */
+
+#ifndef COMMON_UTILS_CALLBACK_GUARD_HPP
+#define COMMON_UTILS_CALLBACK_GUARD_HPP
+
+
+#include <memory>
+
+
+namespace security_containers {
+namespace utils {
+
+/**
+ * Callback guard.
+ * An utility class to control and/or monitor callback lifecycle.
+ */
+class CallbackGuard {
+public:
+    typedef std::shared_ptr<void> Tracker;
+
+    /**
+     * Creates a guard.
+     */
+    CallbackGuard();
+
+    /**
+     * Waits for all trackers.
+     */
+    ~CallbackGuard();
+
+    /**
+     * Creates a tracker.
+     */
+    Tracker spawn() const;
+
+    /**
+     * Gets trackers count
+     */
+    long getTrackersCount() const;
+
+    /**
+     * Wait for all trackers.
+     */
+    bool waitForTrackers(const unsigned int timeoutMs);
+private:
+    class SharedState;
+    std::shared_ptr<SharedState> mSharedState;
+
+    CallbackGuard(const CallbackGuard&) = delete;
+    CallbackGuard& operator=(const CallbackGuard&) = delete;
+};
+
+} // namespace utils
+} // namespace security_containers
+
+
+#endif // COMMON_UTILS_CALLBACK_GUARD_HPP
diff --git a/common/utils/callback-wrapper.hpp b/common/utils/callback-wrapper.hpp
new file mode 100644 (file)
index 0000000..e6707c9
--- /dev/null
@@ -0,0 +1,92 @@
+/*
+ *  Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ *  Contact: Piotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License
+ */
+
+/**
+ * @file
+ * @author  Piotr Bartosiewicz (p.bartosiewi@partner.samsung.com)
+ * @brief   Callback wrapper
+ */
+
+#ifndef COMMON_UTILS_CALLBACK_WRAPPER_HPP
+#define COMMON_UTILS_CALLBACK_WRAPPER_HPP
+
+#include "callback-guard.hpp"
+
+
+namespace security_containers {
+namespace utils {
+
+
+/**
+ * Wraps callback and callback tracker into single object
+ */
+template<class Callback>
+class CallbackWrapper {
+public:
+    CallbackWrapper(const Callback& callback, const CallbackGuard::Tracker& tracker)
+        : mCallback(callback)
+        , mTracker(tracker)
+    {
+    }
+
+    /**
+     * @return Wrapped callback
+     */
+    const Callback& get() const
+    {
+        return mCallback;
+    }
+private:
+    Callback mCallback;
+    CallbackGuard::Tracker mTracker;
+};
+
+/**
+ * Creates callback wrapper. Usefull for C callback api.
+ */
+template<class Callback>
+CallbackWrapper<Callback>* createCallbackWrapper(const Callback& callback,
+                                                 const CallbackGuard::Tracker& tracker)
+{
+    return new CallbackWrapper<Callback>(callback, tracker);
+}
+
+/**
+ * Deletes callback wrapper. Usefull for C callback api.
+ */
+template<class Callback>
+void deleteCallbackWrapper(void* pointer)
+{
+    delete reinterpret_cast<CallbackWrapper<Callback>*>(pointer);
+}
+
+/**
+ * Recovers callback from wrapper pointer. Usefull for C callback api.
+ */
+template<class Callback>
+const Callback& getCallbackFromPointer(const void* pointer)
+{
+    return reinterpret_cast<const CallbackWrapper<Callback>*>(pointer)->get();
+}
+
+
+} // namespace utils
+} // namespace security_containers
+
+
+#endif // COMMON_UTILS_CALLBACK_WRAPPER_HPP
diff --git a/unit_tests/utils/ut-callback-guard.cpp b/unit_tests/utils/ut-callback-guard.cpp
new file mode 100644 (file)
index 0000000..429ed79
--- /dev/null
@@ -0,0 +1,95 @@
+/*
+ *  Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ *  Contact: Piotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License
+ */
+
+
+/**
+ * @file
+ * @author  Piotr Bartosiewicz (p.bartosiewi@partner.samsung.com)
+ * @brief   Unit tests of callback guard
+ */
+
+#include "ut.hpp"
+
+#include "utils/callback-guard.hpp"
+#include "utils/latch.hpp"
+
+#include <future>
+#include <thread>
+
+
+BOOST_AUTO_TEST_SUITE(CallbackGuardSuite)
+
+using namespace security_containers::utils;
+
+const int unsigned TIMEOUT = 1000;
+
+BOOST_AUTO_TEST_CASE(EmptyTest)
+{
+    CallbackGuard guard;
+    BOOST_CHECK_EQUAL(0, guard.getTrackersCount());
+    BOOST_CHECK(guard.waitForTrackers(TIMEOUT));
+}
+
+BOOST_AUTO_TEST_CASE(SimpleTest)
+{
+    CallbackGuard guard;
+    guard.spawn();
+    guard.spawn();
+    BOOST_CHECK_EQUAL(0, guard.getTrackersCount());
+    CallbackGuard::Tracker tracker1 = guard.spawn();
+    CallbackGuard::Tracker tracker2 = guard.spawn();
+    BOOST_CHECK_EQUAL(2, guard.getTrackersCount());
+    CallbackGuard::Tracker tracker2Copy = tracker2;
+    BOOST_CHECK_EQUAL(2, guard.getTrackersCount());
+    tracker2.reset();
+    BOOST_CHECK_EQUAL(2, guard.getTrackersCount());
+    tracker2Copy.reset();
+    BOOST_CHECK_EQUAL(1, guard.getTrackersCount());
+    tracker1.reset();
+    BOOST_CHECK_EQUAL(0, guard.getTrackersCount());
+    BOOST_CHECK(guard.waitForTrackers(TIMEOUT));
+}
+
+BOOST_AUTO_TEST_CASE(ThreadTest)
+{
+    CallbackGuard guard;
+    Latch trackerCreated;
+    Latch trackerCanBeDestroyed;
+
+    std::future<bool> future = std::async(std::launch::async, [&] {
+            CallbackGuard::Tracker tracker = guard.spawn();
+            trackerCreated.set();
+            if (!trackerCanBeDestroyed.wait(TIMEOUT)) {
+                return false;
+            }
+            std::this_thread::sleep_for(std::chrono::milliseconds(200));
+            return true;
+        });
+
+    BOOST_CHECK(trackerCreated.wait(TIMEOUT));
+    BOOST_CHECK_EQUAL(1, guard.getTrackersCount());
+
+    trackerCanBeDestroyed.set();
+    BOOST_CHECK(guard.waitForTrackers(TIMEOUT));
+    BOOST_CHECK_EQUAL(0, guard.getTrackersCount());
+
+    future.wait();
+    BOOST_CHECK(future.get());
+}
+
+BOOST_AUTO_TEST_SUITE_END()