Merge pull request #9 from leapmotion/bug-syncget

AutoPacket::Get synchronous acquisition error
leapmotion · Jul 30, 2014 · 047f627 · 047f627
2 parents 996813c + 69f2300
commit 047f627
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 5 deletions.
diff --git a/autowiring/AutoPacket.h b/autowiring/AutoPacket.h
@@ -148,6 +148,7 @@ class AutoPacket:
   /// </summary>
   template<class T>
   bool Get(const T*& out) const {
+    std::lock_guard<std::mutex> lk(m_lock);
     static_assert(!std::is_same<T, AutoPacket>::value, "Cannot decorate a packet with another packet");
 
     auto q = m_decorations.find(typeid(T));
@@ -178,6 +179,7 @@ class AutoPacket:
   /// </summary>
   template<class T>
   bool Get(const std::shared_ptr<T>*& out) const {
+    std::lock_guard<std::mutex> lk(m_lock);
     auto q = m_decorations.find(typeid(T));
     if(q != m_decorations.end() && q->second.satisfied) {
       auto& disposition = q->second;
@@ -216,13 +218,9 @@ class AutoPacket:
         throw std::runtime_error("Cannot decorate this packet with type T, the requested decoration already exists");
       if(entry.isCheckedOut)
         throw std::runtime_error("Cannot check out this decoration, it's already checked out elsewhere");
+
       entry.isCheckedOut = true;
       entry.wasCheckedOut = true;
-    }
-
-    // Have to find the entry _again_ within the context of a lock and satisfy it here:
-    {
-      std::lock_guard<std::mutex> lk(m_lock);
       m_decorations[typeid(type)].m_decoration = ptr;
     }
 

diff --git a/autowiring/DecorationDisposition.h b/autowiring/DecorationDisposition.h
@@ -10,6 +10,11 @@ struct SatCounter;
 /// </remarks>
 struct DecorationDisposition
 {
+  DecorationDisposition(const DecorationDisposition&) = delete;
+  DecorationDisposition(DecorationDisposition&&) = delete;
+  void operator=(const DecorationDisposition&) = delete;
+  void operator=(DecorationDisposition&&) = delete;
+
   DecorationDisposition(void) :
     m_pImmediate(nullptr),
     m_publisher(nullptr),

diff --git a/src/autowiring/AutoPacket.cpp b/src/autowiring/AutoPacket.cpp
@@ -185,5 +185,6 @@ void AutoPacket::Initialize(void) {
 }
 
 bool AutoPacket::HasSubscribers(const std::type_info& ti) const {
+  std::lock_guard<std::mutex> lk(m_lock);
   return m_decorations.count(ti) != 0;
 }
diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp
@@ -856,3 +856,33 @@ TEST_F(AutoFilterTest, SharedPointerAliasingRules) {
   ASSERT_EQ(1UL, genFilter1->m_called) << "AutoFilter accepting a shared pointer was not called as expected";
   ASSERT_EQ(1UL, genFilter2->m_called) << "AutoFilter accepting a decorated type was not called as expected";
 }
+
+TEST_F(AutoFilterTest, GetSharedPointer) {
+  AutoCurrentContext()->Initiate();
+
+  // Attach a simple decoration
+  AutoRequired<AutoPacketFactory> factory;
+  auto packet = factory->NewPacket();
+  packet->Decorate(Decoration<0>());
+
+  // Verify we can get this decoration back
+  const std::shared_ptr<Decoration<0>>* dec;
+  packet->Get(dec);
+  ASSERT_TRUE(dec != nullptr) << "Failed to obtain a shared pointer for a decoration that was just added";
+
+  // Now add a bunch of other decorations:
+  packet->Decorate(Decoration<1>());
+  packet->Decorate(Decoration<2>());
+  packet->Decorate(Decoration<3>());
+  packet->Decorate(Decoration<4>());
+  packet->Decorate(Decoration<5>());
+  packet->Decorate(Decoration<6>());
+  packet->Decorate(Decoration<7>());
+  packet->Decorate(Decoration<8>());
+  packet->Decorate(Decoration<9>());
+
+  // Verify nothing moved:
+  const std::shared_ptr<Decoration<0>>* dec2;
+  packet->Get(dec2);
+  ASSERT_EQ(dec, dec2) << "Decoration was moved incorrectly after updates were made";
+}