Merge pull request #866 from leapmotion/fix-transientunsatisfy

Fix transitive unsatisfiability defect

src/autowiring/AutoPacket.cpp

@@ -349,11 +349,10 @@ void AutoPacket::UpdateSatisfactionUnsafe(std::unique_lock<std::mutex> lk, const
   // Mark all unsatisfiable output types
   for (auto unsatOutputArg : unsatOutputArgs) {
     // One more producer run, even though we couldn't attach any new decorations
-    auto& entry = m_decoration_map[DecorationKey{unsatOutputArg->id, 0}];
-    entry.m_nProducersRun++;
-
-    // Now recurse on this entry
-    UpdateSatisfactionUnsafe(std::unique_lock<std::mutex>{m_lock}, entry);
+    auto& disposition = m_decoration_map[DecorationKey{unsatOutputArg->id, 0}];
+    if(disposition.IncProducerCount())
+      // Recurse on this entry
+      UpdateSatisfactionUnsafe(std::unique_lock<std::mutex>{m_lock}, disposition);
   }
 }
 
@@ -440,26 +439,11 @@ void AutoPacket::DecorateNoPriors(const AnySharedPointer& ptr, DecorationKey key
     break;
   }
 
-  // Decoration attaches here
-  disposition->m_decorations.push_back(ptr);
-  disposition->m_nProducersRun++;
-
-  // Uniformly advance state:
-  switch (disposition->m_state) {
-  case DispositionState::Unsatisfied:
-  case DispositionState::PartlySatisfied:
-    // Permit a transition to another state
-    if (disposition->IsPublicationComplete()) {
-      disposition->m_state = DispositionState::Complete;
-      UpdateSatisfactionUnsafe(std::move(lk), *disposition);
-    }
-    else
-      disposition->m_state = DispositionState::PartlySatisfied;
-    break;
-  default:
-    // Do nothing, no advancing to any states from here
-    break;
-  }
+  // Decoration attaches here, if it is non-null
+  if(ptr)
+    disposition->m_decorations.push_back(ptr);
+  if(disposition->IncProducerCount())
+    UpdateSatisfactionUnsafe(std::move(lk), *disposition);
 }
 
 void AutoPacket::Decorate(const AnySharedPointer& ptr, DecorationKey key) {
@@ -544,10 +528,13 @@ bool AutoPacket::IsUnsatisfiable(const auto_id& id) const
 {
   const DecorationDisposition* pDisposition = GetDisposition(DecorationKey{ id, 0 });
   if (!pDisposition)
+    // We have never heard of this type
     return false;
   if (!pDisposition->m_decorations.empty())
+    // We have some actual decorations, we know this is not satisfiable
     return false;
-  if (pDisposition->m_nProducersRun == pDisposition->m_publishers.size())
+  if (pDisposition->m_nProducersRun != pDisposition->m_publishers.size())
+    // Some producers have not yet run, we could still feasibly get a decoration back
     return false;
   return true;
 }

src/autowiring/AutoPacket.h

@@ -134,7 +134,7 @@ class AutoPacket:
   /// Marks the specified entry as being unsatisfiable
   /// </summary>
   void MarkUnsatisfiable(const DecorationKey& key);
-  
+
   /// <summary>
   /// Marks timeshifted decorations on successor packets as unsatisfiable
   /// </summary>
@@ -471,18 +471,15 @@ class AutoPacket:
   template<class T>
   void Decorate(const std::shared_ptr<T>& ptr) {
     DecorationKey key(auto_id_t<T>{}, 0);
-    
+
     // We don't want to see this overload used on a const T
     static_assert(!std::is_const<T>::value, "Cannot decorate a shared pointer to const T with this overload");
 
     // Injunction to prevent existential loops:
     static_assert(!std::is_same<T, AutoPacket>::value, "Cannot decorate a packet with another packet");
 
-    // Either decorate, or prevent anyone from decorating
-    if (ptr)
-      Decorate(AnySharedPointer(ptr), key);
-    else
-      MarkUnsatisfiable(key);
+    // Either decorate or abdicate
+    Decorate(AnySharedPointer(ptr), key);
   }
 
   /// <summary>
@@ -777,7 +774,7 @@ class AutoPacket:
   template<typename Fx, typename... InArgs, typename... Outputs>
   void Call(Fx&& fx, void (Fx::*memfn)(InArgs...) const, Outputs&... outputs) {
     typedef typename make_index_tuple<Decompose<decltype(&Fx::operator())>::N>::type t_index;
-    
+
     // Completely unnecessary.  Call will avoid making unneeded copies, and this is guaranteed
     // by a unit test.  Dereference your shared pointers before passing them in.
     static_assert(

src/autowiring/AutoPacketGraph.cpp

@@ -6,6 +6,7 @@
 #include "demangle.h"
 #include "SatCounter.h"
 #include <algorithm>
+#include <cassert>
 #include <fstream>
 #include <string>
 #include <sstream>

src/autowiring/BasicThread.cpp

@@ -9,6 +9,7 @@
 #include "dispatch_aborted_exception.h"
 #include "GlobalCoreContext.h"
 #include "fast_pointer_cast.h"
+#include <cassert>
 #include ATOMIC_HEADER
 
 static auto mainTID = std::this_thread::get_id();

src/autowiring/CallExtractor.h

@@ -121,7 +121,7 @@ struct CE<void (T::*)(Args...) const, index_tuple<N...>> :
 {
   typedef CESetup<Args...> t_ceSetup;
   static const bool deferred = false;
-  
+
   template<void(T::*memFn)(Args...) const>
   static void Call(const void* pObj, AutoPacket& packet) {
     // Extract, call, commit

src/autowiring/CoreContext.cpp

@@ -11,6 +11,7 @@
 #include "NullPool.h"
 #include "SystemThreadPool.h"
 #include "thread_specific_ptr.h"
+#include <cassert>
 #include <sstream>
 #include <stdexcept>
 

src/autowiring/DecorationDisposition.h

@@ -3,7 +3,7 @@
 #include "AutowiringConfig.h"
 #include "altitude.h"
 #include "AnySharedPointer.h"
-#include <cassert>
+#include <atomic>
 #include <set>
 #include <vector>
 
@@ -126,15 +126,40 @@ struct DecorationDisposition
   // The current state of this disposition
   DispositionState m_state = DispositionState::Unsatisfied;
 
+  /// <summary>
+  /// Increments the number of producers run by one
+  /// </summary>
+  /// <returns>True if the decoration has become satisfied as a result of this call</returns>
+  bool IncProducerCount(void) {
+    auto producersRun = ++m_nProducersRun;
+
+    // Uniformly advance state:
+    switch (m_state) {
+    case DispositionState::Unsatisfied:
+    case DispositionState::PartlySatisfied:
+      // Permit a transition to another state
+      if (producersRun >= m_publishers.size()) {
+        m_state = DispositionState::Complete;
+        return true;
+      }
+
+      m_state = DispositionState::PartlySatisfied;
+      break;
+    default:
+      // Do nothing, no advancing to any states from here
+      break;
+    }
+    return false;
+  }
+
   /// <returns>
   /// True if all publishers have run on this disposition
   /// </summary>
   /// <remarks>
-  /// Publication is complete automatically on this type if there are no statically known publishers,
-  /// and at least one decoration has been attached to this disposition.
+  /// Publication is complete whenever all producers have run on the decoration.
   /// </remarks>
   bool IsPublicationComplete(void) const {
-    return !m_decorations.empty() && m_nProducersRun >= m_publishers.size();
+    return m_nProducersRun >= m_publishers.size();
   }
 
   void Reset(void) {

src/autowiring/test/AutoFilterSatisfiabilityTest.cpp

@@ -0,0 +1,63 @@
+// Copyright (C) 2012-2015 Leap Motion, Inc. All rights reserved.
+#include "stdafx.h"
+#include <autowiring/autowiring.h>
+#include "TestFixtures/Decoration.hpp"
+
+class AutoFilterSatisfiabilityTest :
+  public testing::Test
+{
+public:
+  AutoFilterSatisfiabilityTest(void) {
+    AutoCurrentContext()->Initiate();
+  }
+
+  AutoRequired<AutoPacketFactory> factory;
+};
+
+TEST_F(AutoFilterSatisfiabilityTest, MarkUnsatisfiableCalls) {
+  auto packet = factory->NewPacket();
+
+  // This filter shouldn't be called, because it expects a reference as an input
+  bool bRefCalled = false;
+  *packet += [&bRefCalled](const Decoration<0>& dec) { bRefCalled = true; };
+
+  // This one should be called because the input is a shared pointer
+  bool bSharedPtrCalled = false;
+  *packet += [&bSharedPtrCalled](std::shared_ptr<const Decoration<0>> dec) { bSharedPtrCalled = true; };
+
+  packet->MarkUnsatisfiable<Decoration<0>>();
+
+  ASSERT_FALSE(bRefCalled) << "Reference version should not have been called";
+  ASSERT_TRUE(bSharedPtrCalled) << "Shared pointer version should have been called as a result of Unsatisfiable";
+}
+
+TEST_F(AutoFilterSatisfiabilityTest, TransitiveUnsatisfiability) {
+  // Set up the filter configuration that will create the transitive condition
+  auto called0 = std::make_shared<bool>(false);
+  *factory += [called0](Decoration<0> in, std::shared_ptr<Decoration<1>>& out) {
+    *called0 = true;
+  };
+
+  // This filter accepts Decoration<1> as an optional input argument.  It should be called
+  // with this value set to nullptr.
+  auto called1 = std::make_shared<bool>(false);
+  *factory += [called1](std::shared_ptr<const Decoration<1>> in) {
+    *called1 = true;
+  };
+
+  // This filter won't be called at all
+  *factory += [] (Decoration<1>, Decoration<2>&) {};
+
+  // This verifies that we do have correct transitive unsatisfiability behavior
+  auto called2 = std::make_shared<bool>(false);
+  *factory += [called2] (std::shared_ptr<const Decoration<2>>) {
+    *called2 = true;
+  };
+
+  auto packet = factory->NewPacket();
+  packet->Decorate(Decoration<0>{});
+  ASSERT_TRUE(packet->IsUnsatisfiable<Decoration<1>>());
+  ASSERT_TRUE(*called1);
+  ASSERT_TRUE(packet->IsUnsatisfiable<Decoration<2>>());
+  ASSERT_TRUE(*called2);
+}

src/autowiring/test/CMakeLists.txt

@@ -12,6 +12,7 @@ set(AutowiringTest_SRCS
   AutoFilterFunctionTest.cpp
   AutoFilterMultiDecorateTest.cpp
   AutoFilterRvalueTest.cpp
+  AutoFilterSatisfiabilityTest.cpp
   AutoFilterSequencing.cpp
   AutoFilterTest.cpp
   AutoIDTest.cpp
@@ -54,7 +55,6 @@ set(AutowiringTest_SRCS
   SpinLockTest.cpp
   TeardownNotifierTest.cpp
   TypeRegistryTest.cpp
-  SatisfiabilityTest.cpp
   ScopeTest.cpp
   SnoopTest.cpp
   ThreadPoolTest.cpp