Merge pull request #24 from leapmotion/ref-lifecycletest

Adding additional constraints to validate object lifecycle in ObjectPool

src/autowiring/AutoPacket.cpp

@@ -73,73 +73,94 @@ ObjectPool<AutoPacket> AutoPacket::CreateObjectPool(AutoPacketFactory& factory,
 }
 
 void AutoPacket::MarkUnsatisfiable(const std::type_info& info) {
+  std::list<SatCounter*> callQueue;
   DecorationDisposition* decoration;
   {
     std::lock_guard<std::mutex> lk(m_lock);
     auto dFind = m_decorations.find(info);
     if(dFind == m_decorations.end())
       // Trivial return, there's no subscriber to this decoration and so we have nothing to do
       return;
+
+    // Update satisfaction inside of lock
     decoration = &dFind->second;
+    for(auto& satCounter : decoration->m_subscribers) {
+      if(satCounter.second)
+        // Entry is mandatory, leave it unsatisfaible
+        continue;
+
+      // Entry is optional, we will call if we're satisfied after decrementing this optional field
+      if(satCounter.first->Decrement(false))
+        callQueue.push_back(satCounter.first);
+    }
   }
 
-  // Update everything
-  for(auto& satCounter : decoration->m_subscribers) {
-    if(satCounter.second)
-      // Entry is mandatory, leave it unsatisfaible
-      continue;
-
-    // Entry is optional, we will call if we're satisfied after decrementing this optional field
-    if(satCounter.first->Decrement(false))
-        satCounter.first->CallAutoFilter(*this);
-  }
+  // Make calls outside of lock, to avoid deadlock from decorations in methods
+  for (SatCounter* call : callQueue)
+    call->CallAutoFilter(*this);
 }
 
 void AutoPacket::UpdateSatisfaction(const std::type_info& info) {
+  std::list<SatCounter*> callQueue;
   DecorationDisposition* decoration;
   {
     std::lock_guard<std::mutex> lk(m_lock);
     auto dFind = m_decorations.find(info);
     if(dFind == m_decorations.end())
       // Trivial return, there's no subscriber to this decoration and so we have nothing to do
       return;
+
+    // Update satisfaction inside of lock
     decoration = &dFind->second;
+    for(auto& satCounter : decoration->m_subscribers)
+      if(satCounter.first->Decrement(satCounter.second))
+        callQueue.push_back(satCounter.first);
   }
 
-  // Update everything
-  for(auto& satCounter : decoration->m_subscribers)
-    if(satCounter.first->Decrement(satCounter.second))
-        satCounter.first->CallAutoFilter(*this);
+  // Make calls outside of lock, to avoid deadlock from decorations in methods
+  for (SatCounter* call : callQueue)
+    call->CallAutoFilter(*this);
 }
 
 void AutoPacket::PulseSatisfaction(DecorationDisposition* pTypeSubs[], size_t nInfos) {
+  std::list<SatCounter*> callQueue;
   // First pass, decrement what we can:
-  for(size_t i = nInfos; i--;) {
-    for(std::pair<SatCounter*, bool>& subscriber : pTypeSubs[i]->m_subscribers) {
-      SatCounter* cur = subscriber.first;
-      if(
-        // We only care about mandatory inputs
-        subscriber.second &&
-
-        // We only care about sat counters that aren't deferred--skip everyone else
-        // Deferred calls will be too late.
-        !cur->IsDeferred() &&
-
-        // Now do the decrementation, and only proceed if the decremented value is zero
-        !--cur->remaining
-      )
-        // Finally, a call is safe to make on this type
-        cur->CallAutoFilter(*this);
+  {
+    std::lock_guard<std::mutex> lk(m_lock);
+    for(size_t i = nInfos; i--;) {
+      for(std::pair<SatCounter*, bool>& subscriber : pTypeSubs[i]->m_subscribers) {
+        SatCounter* cur = subscriber.first;
+        if(
+          // We only care about mandatory inputs
+          subscriber.second &&
+
+          // We only care about sat counters that aren't deferred--skip everyone else
+          // Deferred calls will be too late.
+          !cur->IsDeferred() &&
+
+          // Now do the decrementation, and only proceed if the decremented value is zero
+          !--cur->remaining
+        )
+          // Finally, queue a call for this type
+          callQueue.push_back(cur);
+      }
     }
   }
 
+  // Make calls outside of lock, to avoid deadlock from decorations in methods
+  for (SatCounter* call : callQueue)
+    call->CallAutoFilter(*this);
+
   // Reset all counters
   // since data in this call will not be available subsequently
-  for(size_t i = nInfos; i--;) {
-    for(auto& satCounter : pTypeSubs[i]->m_subscribers) {
-      auto& cur = satCounter.first;
-      if (satCounter.second) {
-        ++cur->remaining;
+  {
+    std::lock_guard<std::mutex> lk(m_lock);
+    for(size_t i = nInfos; i--;) {
+      for(auto& satCounter : pTypeSubs[i]->m_subscribers) {
+        auto& cur = satCounter.first;
+        if (satCounter.second) {
+          ++cur->remaining;
+        }
       }
     }
   }

src/autowiring/test/ObjectPoolTest.cpp

@@ -21,6 +21,104 @@ TEST_F(ObjectPoolTest, VerifyOutstandingLimit) {
   EXPECT_TRUE(obj3 == nullptr) << "Object pool issued more objects than it was authorized to issue";
 }
 
+class LifeCycle {
+  //Lock ensures that status checks are atomic.
+  //This does not prevent asynchronous calls.
+  std::mutex lock;
+  enum LifeStage {
+    construct,
+    pooled,
+    issued,
+    destruct
+  } stage;
+
+public:
+  LifeCycle() : stage(construct) {
+    std::lock_guard<std::mutex> guard(lock);
+    if (stage == construct)
+      stage = pooled;
+    else
+      throw std::runtime_error("Initialization interrupted");
+  }
+
+  ~LifeCycle() {
+    lock.lock();
+    if (stage != pooled)
+      throw std::runtime_error("Destructor called before Finalize");
+    lock.unlock();
+  }
+
+  static ObjectPool<LifeCycle>* NewObjectPool(size_t limit = ~0, size_t maxPooled = ~0) {
+    return new ObjectPool<LifeCycle>(limit, maxPooled,
+      [] { return new LifeCycle(); },
+      [] (LifeCycle& life) { life.Initialize(); },
+      [] (LifeCycle& life) { life.Finalize(); }
+    );
+  }
+
+protected:
+  void Initialize() {
+    std::lock_guard<std::mutex> guard(lock);
+    if (stage != pooled)
+      throw std::runtime_error("Initialize called on object not pooled");
+    stage = issued;
+  }
+
+  void Finalize() {
+    std::lock_guard<std::mutex> guard(lock);
+    if (stage != issued)
+      throw std::runtime_error("Finalize called on object not issued");
+    stage = pooled;
+  }
+};
+
+TEST_F(ObjectPoolTest, LifeCycleTestLimitOne) {
+  std::shared_ptr<ObjectPool<LifeCycle>> pool(LifeCycle::NewObjectPool(2, 2));
+  std::shared_ptr<LifeCycle> objHold, objDrop;
+
+  // Create in Pool
+  try {
+    pool->Preallocate(1);
+  } catch (std::runtime_error e) {
+    FAIL() << e.what();
+  }
+
+  // Issue from Pool
+  try {
+    (*pool)(objHold);
+  } catch (std::runtime_error e) {
+    FAIL() << e.what();
+  }
+
+  // Issue from Pool with implicit Creation of objDrop
+  try {
+    (*pool)(objDrop);
+  } catch (std::runtime_error e) {
+    FAIL() << e.what();
+  }
+
+  // Return to Pool
+  try {
+    objDrop.reset();
+  } catch (std::runtime_error e) {
+    FAIL() << e.what();
+  }
+
+  // Destroy Pool with implicit Destruction of objDrop
+  try {
+    pool.reset();
+  } catch (std::runtime_error e) {
+    FAIL() << e.what();
+  }
+
+  // Return to Pool redirected to Destruction of objHold
+  try {
+    objHold.reset();
+  } catch (std::runtime_error e) {
+    FAIL() << e.what();
+  }
+}
+
 TEST_F(ObjectPoolTest, VerifyAsynchronousUsage) {
   AutoCreateContext ctxt;
   CurrentContextPusher pshr(ctxt);