Fixed memleak when cancelling pending calls

martinhaefner · Nov 24, 2017 · 58aaa70 · 58aaa70
1 parent 4fff93a
commit 58aaa70
Show file tree

Hide file tree

Showing 13 changed files with 52 additions and 32 deletions.
diff --git a/examples/echo/main.cpp b/examples/echo/main.cpp
@@ -30,6 +30,8 @@ class MyEchoClient : public simppl::dbus::Stub<simppl::example::EchoService>
                 }
                 else
                     std::cout << "Got error: " << st.what() << std::endl;
+
+                disp().stop();
             };
         }
     }

diff --git a/include/simppl/clientside.h b/include/simppl/clientside.h
@@ -2,7 +2,6 @@
 #define SIMPPL_CLIENTSIDE_H
 
 
-#include <iostream>
 #include <functional>
 
 #include <dbus/dbus.h>
@@ -275,7 +274,7 @@ ClientProperty<DataT, Flags>& ClientProperty<DataT, Flags>::attach()
 		this->f_(CallState(42), *d.template get<data_type>());
   });
 
-  dbus_pending_call_set_notify(dbus_pending_call_ref(stub().get_property_async(name_).pending()),
+  dbus_pending_call_set_notify(stub().get_property_async(name_).pending(),
 	 &holder_type::pending_notify,
 	 new holder_type([this](CallState cs, const arg_type& val){
 		if (f_)
@@ -388,7 +387,7 @@ inline
 simppl::dbus::PendingCall operator>>(simppl::dbus::detail::InterimCallbackHolder<HolderT>&& r, const FunctorT& f)
 {
    // TODO static_assert FunctorT and HolderT::f_ convertible?
-   dbus_pending_call_set_notify(dbus_pending_call_ref(r.pc_.pending()), &HolderT::pending_notify, new HolderT(f), &HolderT::_delete);
+   dbus_pending_call_set_notify(r.pc_.pending(), &HolderT::pending_notify, new HolderT(f), &HolderT::_delete);
 
    return std::move(r.pc_);
 }

diff --git a/include/simppl/detail/holders.h b/include/simppl/detail/holders.h
@@ -19,13 +19,21 @@ struct InterimCallbackHolder
 {
    typedef HolderT holder_type;
 
+   InterimCallbackHolder(const InterimCallbackHolder& rhs)
+   : pc_(std::move(rhs.pc_))
+   {
+	   // NOOP
+   }
+
+   InterimCallbackHolder& operator=(const InterimCallbackHolder&) = delete;
+
    explicit inline
-   InterimCallbackHolder(PendingCall&& pc)
+   InterimCallbackHolder(const PendingCall& pc)
     : pc_(std::move(pc))
    {
       // NOOP
    }
-
+   
    PendingCall pc_;
 };
 
@@ -39,11 +47,11 @@ struct CallbackHolder
 
    explicit inline
    CallbackHolder(const FuncT& f)
-    : f_(f)
+    : f_(std::move(f))
    {
       // NOOP
    }
-
+   
    static inline
    void _delete(void* p)
    {
@@ -54,7 +62,6 @@ struct CallbackHolder
    static
    void pending_notify(DBusPendingCall* pc, void* data)
    {
-       auto upc = simppl::dbus::make_pending_call(pc);
        auto msg = simppl::dbus::make_message(dbus_pending_call_steal_reply(pc));
 
        auto that = (CallbackHolder*)data;
@@ -96,8 +103,7 @@ struct PropertyCallbackHolder
    static
    void pending_notify(DBusPendingCall* pc, void* data)
    {
-       auto upc = simppl::dbus::make_pending_call(pc);
-       auto msg = simppl::dbus::make_message(dbus_pending_call_steal_reply(pc));
+	   auto msg = simppl::dbus::make_message(dbus_pending_call_steal_reply(pc));
 
        auto that = (PropertyCallbackHolder*)data;
        assert(that->f_);

diff --git a/include/simppl/detail/serialization.h b/include/simppl/detail/serialization.h
@@ -9,7 +9,6 @@
 #include "simppl/objectpath.h"
 #include <cxxabi.h>
 
-#include <iostream>
 #include <map>
 #include <vector>
 #include <tuple>
@@ -875,10 +874,7 @@ struct Deserializer // : noncopyable
        Deserializer s(&iter);
 
        if (!try_deserialize(s, v, dbus_message_iter_get_signature(&iter)))
-       {
-          std::cerr << "Invalid variant type detected" << std::endl;
           assert(false);
-       }
 
        dbus_message_iter_next(iter_);
        return *this;

diff --git a/include/simppl/pendingcall.h b/include/simppl/pendingcall.h
@@ -16,14 +16,12 @@ struct PendingCall
     PendingCall(uint32_t serial, DBusPendingCall* p);
     PendingCall();
 
-    // only move
     PendingCall(PendingCall&& rhs);
     PendingCall& operator=(PendingCall&& rhs);
 
-    // no copy
-    PendingCall(const PendingCall&) = delete;
-    PendingCall& operator=(const PendingCall&) = delete;
-
+    PendingCall(const PendingCall& rhs);
+    PendingCall& operator=(const PendingCall& rhs);
+
     /** request serial */
     uint32_t serial() const;
 

diff --git a/include/simppl/serverside.h b/include/simppl/serverside.h
@@ -1,8 +1,6 @@
 #ifndef SIMPPL_SERVERSIDE_H
 #define SIMPPL_SERVERSIDE_H
 
-#include <set>
-#include <iostream>
 
 #include <dbus/dbus.h>
 

diff --git a/include/simppl/skeletonbase.h b/include/simppl/skeletonbase.h
@@ -5,8 +5,6 @@
 
 #include <dbus/dbus.h>
 
-#include <iostream>  // FIXME remove this
-
 #include "simppl/error.h"
 #include "simppl/serverrequestdescriptor.h"
 

diff --git a/include/simppl/types.h b/include/simppl/types.h
@@ -3,7 +3,7 @@
 
 
 #include <memory>
-#include <iostream>
+
 #include <dbus/dbus.h>
 
 

diff --git a/src/dispatcher.cpp b/src/dispatcher.cpp
@@ -5,6 +5,7 @@
 
 #include <unistd.h>
 
+#include <iostream>
 #include <map>
 #include <set>
 #include <atomic>
@@ -488,6 +489,7 @@ void Dispatcher::add_server(SkeletonBase& serv)
 
    if (dbus_error_is_set(&err))
    {
+	   // FIXME make exception classes
       std::cerr << "dbus_bus_request_name - DBus error: " << err.name << ": " << err.message << std::endl;
       dbus_error_free(&err);
    }

diff --git a/src/error.cpp b/src/error.cpp
@@ -1,6 +1,5 @@
 #include "simppl/error.h"
 
-#include <iostream>
 #include <cassert>
 #include <cstring>
 

diff --git a/src/pendingcall.cpp b/src/pendingcall.cpp
@@ -13,7 +13,7 @@ namespace dbus
 PendingCall::PendingCall(uint32_t serial, DBusPendingCall* p)
  : serial_(serial)
 {
-    if (p)
+	if (p)
         pending_ = make_pending_call(p);
 }
 
@@ -27,7 +27,7 @@ PendingCall::PendingCall()
 
 PendingCall::PendingCall(PendingCall&& rhs)
 {
-    serial_ = rhs.serial_;
+	serial_ = rhs.serial_;
     pending_ = rhs.pending_;
 
     rhs.reset();
@@ -36,14 +36,33 @@ PendingCall::PendingCall(PendingCall&& rhs)
 
 PendingCall& PendingCall::operator=(PendingCall&& rhs)
 {
-    serial_ = rhs.serial_;
+	serial_ = rhs.serial_;
     pending_ = rhs.pending_;
 
     rhs.reset();
     return *this;
 }
 
 
+PendingCall::PendingCall(const PendingCall& rhs)
+ : serial_(rhs.serial_)
+ , pending_(rhs.pending_)
+{
+	// NOOP
+}
+
+
+PendingCall& PendingCall::operator=(const PendingCall& rhs)
+{
+	if (&rhs != this)
+	{
+		serial_ = rhs.serial_;
+		pending_ = rhs.pending_;
+	}
+
+	return *this; 
+}
+
 uint32_t PendingCall::serial() const
 {
     return serial_;
@@ -60,9 +79,9 @@ DBusPendingCall* PendingCall::pending()
 void PendingCall::cancel()
 {
     if (pending_)
-        dbus_pending_call_cancel(pending_.get());
-
-    reset();
+		dbus_pending_call_cancel(pending_.get());
+	
+	reset();
 }
 
 

diff --git a/src/skeletonbase.cpp b/src/skeletonbase.cpp
@@ -9,6 +9,8 @@
 
 #include "simppl/detail/util.h"
 
+#include <iostream>
+
 
 #if 0
 namespace org

diff --git a/tests/simple.cpp b/tests/simple.cpp
@@ -109,8 +109,9 @@ struct CancelClient : simppl::dbus::Stub<Simple>
             };
 
             std::this_thread::sleep_for(100ms);
+
             p.cancel();
-
+            
             // must stop server
             oneway(7777);
          }
-Original file line number
+Diff line change
@@ Expand Up @@
                     }
                     else
                         std::cout << "Got error: " << st.what() << std::endl;
+                    disp().stop();
                 };
             }
         }
@@ Expand Down @@
Original file line number	Diff line number	Diff line change
Expand Up		@@ -3,7 +3,7 @@


		#include <memory>
		#include <iostream>

		#include <dbus/dbus.h>


Expand Down