Merge branch 'feature/notifier_destructor' into 'master'

Add a destructor to Notifier to prevent Segfault w/o extending the Lifetime of Objects via shared_ptr

See merge request amdis/amdis-core!144

src/amdis/DOFVector.hpp

@@ -66,7 +66,7 @@ namespace AMDiS
               DataTransferOperation op = DataTransferOperation::INTERPOLATE)
       : Coefficients(*basis)
       , Observer<event::preAdapt>(basis->gridView().grid())
-      , Observer<event::adapt>(basis)
+      , Observer<event::adapt>(*basis)
       , Observer<event::postAdapt>(basis->gridView().grid())
       , dataTransfer_(op, basis)
       , basis_(basis)
@@ -129,12 +129,12 @@ namespace AMDiS
     }
 
 
-    Coefficients const& coefficients() const 
+    Coefficients const& coefficients() const
     {
       return static_cast<Coefficients const&>(*this);
     }
 
-    Coefficients& coefficients() 
+    Coefficients& coefficients()
     {
       return static_cast<Coefficients&>(*this);
     }

src/amdis/Observer.hpp

 #pragma once
 
-#include <algorithm>
-#include <memory>
 #include <set>
-#include <type_traits>
-
-#include <dune/common/shared_ptr.hh>
-#include <dune/common/typeutilities.hh>
+#include <utility>
 
 #include <amdis/common/ConceptsBase.hpp>
 #include <amdis/common/Index.hpp>
+#include <amdis/common/TypeTraits.hpp>
 
 namespace AMDiS
 {
   namespace event
   {
-    /** 
-     * An event that is signaled before the actual adaption happens. Example: grid.preAdapt(). 
+    /**
+     * An event that is signaled before the actual adaption happens. Example: grid.preAdapt().
      * The \ref value might indicate whether any pre-processing is necessary.
      **/
     struct preAdapt { bool value = true; };
 
-    /** 
+    /**
      * An event that is called directly of the adaption. Example: grid.adapt().
      * The \ref value indicates whether something is changed during adaption.
      **/
     struct adapt { bool value = true; };
 
-    /** 
+    /**
      * An event that is called after adaption to indicate the start of a clean-up phase.
      **/
     struct postAdapt {};
@@ -40,6 +36,7 @@ namespace AMDiS
   public:
     virtual ~ObserverInterface() = default;
     virtual void update(Event e) = 0;
+    virtual void unset() = 0;
   };
 
 
@@ -66,6 +63,13 @@ namespace AMDiS
   class Notifier<Event>
   {
   public:
+    virtual ~Notifier()
+    {
+      // Remove remaining pointers to this to avoid segfaults
+      for (ObserverInterface<Event>* o : observers_)
+        o->unset();
+    }
+
     /// Call the \ref update method on all attached observers.
     void notify(Event const& e)
     {
@@ -90,25 +94,19 @@ namespace AMDiS
   };
 
 
-  /// Implementation of the \ref ObserverInterface 
+  /// Implementation of the \ref ObserverInterface
   template <class Event, class... Tags>
   class Observer
       : public ObserverInterface<Event>
   {
   public:
     template <class Notifier>
-    Observer(std::shared_ptr<Notifier> notifier)
-      : notifier_(std::const_pointer_cast<std::remove_const_t<Notifier>>(std::move(notifier)))
+    Observer(Notifier const& notifier)
+      : notifier_(const_cast<Notifier*>(&notifier))
     {
       notifier_->attach(this);
     }
 
-    template <class Notifier,
-      class = void_t<decltype(std::declval<std::remove_const_t<Notifier>>().notify(std::declval<Event>()))> >
-    Observer(Notifier& notifier)
-      : Observer(Dune::stackobject_to_shared_ptr(notifier))
-    {}
-
     /// Destructor, detaches from the notifier
     virtual ~Observer()
     {
@@ -134,6 +132,13 @@ namespace AMDiS
       return *this;
     }
 
+    /// Set the Notifier* to nullptr. Used by the Notifer to avoid segfaults when destruction occurs
+    /// out of order.
+    void unset() final
+    {
+      notifier_ = nullptr;
+    }
+
     /// Implementation of the interface method \ref ObserverInterface::update.
     /// Redirects to the \ref updateImpl method with additional \ref Tags parameters
     void update(Event e) final
@@ -148,11 +153,11 @@ namespace AMDiS
     virtual void updateImpl(Event e, Tags...) = 0;
 
   private:
-    std::shared_ptr<Notifier<Event>> notifier_ = nullptr;
+    Notifier<Event>* notifier_ = nullptr;
   };
 
 
-  namespace Impl 
+  namespace Impl
   {
     template <class Event, class Tags>
     class ObserverSequenceImpl;

src/amdis/linearalgebra/SparsityPattern.hpp

@@ -9,7 +9,7 @@
 
 namespace AMDiS
 {
-  /// \brief A general sparsity pattern implementation using the full pattern of the 
+  /// \brief A general sparsity pattern implementation using the full pattern of the
   /// basis by adding all local indices
   template <class RowBasis, class ColBasis>
   class SparsityPattern
@@ -88,8 +88,8 @@ namespace AMDiS
 
     void updateImpl3()
     {
-      rowBasis_ == colBasis_ 
-          ? updateSameBasis() 
+      rowBasis_ == colBasis_
+          ? updateSameBasis()
           : updateDifferentBasis();
     }
 

src/amdis/linearalgebra/mtl/SlotSize.hpp

@@ -63,16 +63,16 @@ namespace AMDiS
   protected:
 
     // update of the row basis, just update the dimension
-    void updateImpl(event::adapt e, index_t<0> i) final 
-    { 
-      rows_ = rowBasis_.dimension(); 
+    void updateImpl(event::adapt e, index_t<0> i) final
+    {
+      rows_ = rowBasis_.dimension();
     }
 
-    // estimate the number of columns by multiplying the maximal node size with the 
-    // number of element surrounding a vertex. This number is approximated by the 
+    // estimate the number of columns by multiplying the maximal node size with the
+    // number of element surrounding a vertex. This number is approximated by the
     // number of simplices surrounding a vertex in a kuhn tringulation
-    void updateImpl(event::adapt e, index_t<1> i) final 
-    { 
+    void updateImpl(event::adapt e, index_t<1> i) final
+    {
       cols_ = colBasis_.dimension();
       estRowSize_ = std::min(cols_, colBasis_.maxSize() * surrounding_);
     }

test/ObserverTest.cpp

@@ -62,7 +62,7 @@ class B
     , public Notifier<event::bazEvent>
 {
 public:
-  B(std::shared_ptr<A> a)
+  B(A const& a)
     : Observer<event::fooEvent>(a)
     , Observer<event::bazEvent>(a)
     , a_(a)
@@ -81,9 +81,9 @@ public:
     this->notify(e);
   }
 
-  std::shared_ptr<A> const& a() const { return a_; }
-
-  std::shared_ptr<A> a_;
+  // Provide access to a_. This allows a class with access to B to observe A.
+  A const& a() const { return a_; }
+  A const& a_;
 };
 
 class C
@@ -91,8 +91,8 @@ class C
     , private Observer<event::bazEvent>
 {
 public:
-  C(std::shared_ptr<B> b)
-    : Observer<event::barEvent>(b->a())
+  C(B const& b)
+    : Observer<event::barEvent>(b.a())
     , Observer<event::bazEvent>(b)
   {}
 
@@ -127,7 +127,7 @@ class E
     , public Observer<event::barEvent>
 {
 public:
-  E(std::shared_ptr<A> a, std::shared_ptr<D> d)
+  E(A const& a, D const& d)
       : Observer<event::fooEvent>(a)
       , Observer<event::barEvent>(d)
   {}
@@ -145,51 +145,98 @@ public:
   }
 };
 
-/// Observing a class that inherits from Observer<T, Events...> more than once is not supported
-// class F
-//     : public Observer<E, event::fooEvent>
-// {
-// public:
-//   F(std::shared_ptr<E> e)
-//     : Observer<E, event::fooEvent>(e)  // Error
-//   {}
-// };
+class F
+    : public ObserverSequence<event::barEvent, 2>
+{
+public:
+  F(D const& d1, D const& d2)
+    : ObserverSequence<event::barEvent, 2>(d1, d2)
+  {}
+
+  void updateImpl(event::barEvent, index_t<0> i) override
+  {
+    std::cout << "F::update(bar, 0)\n";
+    testValue += "F.bar0 ";
+  }
+
+  void updateImpl(event::barEvent, index_t<1> i) override
+  {
+    std::cout << "F::update(bar, 1)\n";
+    testValue += "F.bar1 ";
+  }
+};
 
 /// Test the observer hierarchy.
 /**
- *  There are 4 hierarchies tested.
+ *  There are 5 hierarchies tested.
  *  A::foo(): A -> B, E     (multiple observers [B,E] of one event)
- *  A::bar(): A -> (B) -> C (indirect observer [C of A])
- *  A::baz(): A -> B -> C   (simultanious observer and signaller of an event [B])
+ *  A::bar(): A -> (B) -> C (indirect observer [C of A] via access function)
+ *  A::baz(): A -> B -> C   (simultanious observer and notifier of an event [B])
  *  D::bar(): D -> E        (observer of multiple classes [E])
+ *  F::bar(): D1/D2 -> F    (observer of multiple classes with the same event [F])
  */
 int main()
 {
   using namespace AMDiS;
 
-  auto a = std::make_shared<A>();
-  auto b = std::make_shared<B>(a);
-  auto c = std::make_shared<C>(b);
-  auto d = std::make_shared<D>();
-  auto e = std::make_shared<E>(a, d);
+  A a;
   std::string ref, refOpt;
+  {
+    B b(a);
+    C c(b);
+    D d;
+    E e(a, d);
+    D d1;
+    D d2;
+    D d3;
+    F f1(d1, d2);
+    F f2(d3, d3);
+
+    a.foo();
+    ref = "A.foo B.foo E.foo ";
+    refOpt = "A.foo E.foo B.foo ";  // Order of b.foo() and e.foo() unspecified
+    AMDIS_TEST(checkAndResetTestValue("Test: both b and e are notified", ref, refOpt));
+
+    a.bar();
+    ref = "A.bar C.bar ";
+    AMDIS_TEST(checkAndResetTestValue("Test: c is notified", ref));
+
+    a.baz();
+    ref = "A.baz B.baz C.baz ";
+    AMDIS_TEST(checkAndResetTestValue("Test: b and c are notified in order", ref));
+
+    d.bar();
+    ref = "D.bar E.bar ";
+    AMDIS_TEST(checkAndResetTestValue("Test: e is notified and the correct overload is chosen", ref));
+
+    d1.bar();
+    ref = "D.bar F.bar0 ";
+    AMDIS_TEST(checkAndResetTestValue("Test: both d1 and d2 notify f1: d1", ref));
+    d2.bar();
+    ref = "D.bar F.bar1 ";
+    AMDIS_TEST(checkAndResetTestValue("Test: both d1 and d2 notify f1: d2", ref));
+
+    d3.bar();
+    ref = "D.bar F.bar0 F.bar1 ";
+    refOpt = "D.bar F.bar1 F.bar0 ";
+    AMDIS_TEST(checkAndResetTestValue("Test: d3 notifies f2 twice", ref, refOpt));
+  }
 
-  a->foo();
-  ref = "A.foo B.foo E.foo ";
-  refOpt = "A.foo E.foo B.foo ";  // Order of b->foo() and e->foo() unspecified
-  AMDIS_TEST(checkAndResetTestValue("A:foo()", ref, refOpt));
-
-  a->bar();
-  ref = "A.bar C.bar ";
-  AMDIS_TEST(checkAndResetTestValue("A:bar()", ref));
-
-  a->baz();
-  ref = "A.baz B.baz C.baz ";
-  AMDIS_TEST(checkAndResetTestValue("A:baz()", ref));
-
-  d->bar();
-  ref = "D.bar E.bar ";
-  AMDIS_TEST(checkAndResetTestValue("D:bar()", ref));
+  // Observers went out of scope
+  try {
+    a.foo();
+    ref = "A.foo ";
+    AMDIS_TEST(checkAndResetTestValue("Test: observers detach properly: foo", ref));
+    a.bar();
+    ref = "A.bar ";
+    AMDIS_TEST(checkAndResetTestValue("Test: observers detach properly: bar", ref));
+    a.baz();
+    ref = "A.baz ";
+    AMDIS_TEST(checkAndResetTestValue("Test: observers detach properly: baz", ref));
+  } catch(...) {
+    std::cout << "Observer did not detach properly and threw an exception\n";
+    Impl::num_errors()++;
+  }
 
   return report_errors();
 }