From 4fa7fb5e0fdc3112c12eccf5f92c7ab6ee1d67e5 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Tue, 15 Jul 2014 16:41:12 -0700 Subject: [PATCH 01/17] Added test for nullptr decoration. --- src/autowiring/test/AutoFilterTest.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index daf74c85d..d88be2e6c 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -160,6 +160,18 @@ TEST_F(AutoFilterTest, VerifyNoMultiDecorate) { EXPECT_LT(0, filterA->m_called) << "Filter was not called after being fully satisfied"; } +TEST_F(AutoFilterTest, VerifyNoNullCheckout) { + AutoCurrentContext()->Initiate(); + AutoRequired factory; + + std::shared_ptr> nulldeco; + ASSERT_FALSE(nulldeco); + + auto packet = factory->NewPacket(); + EXPECT_THROW(packet->Checkout(nulldeco), std::exception) << "Failed to catch null checkout" << std::endl; + EXPECT_THROW(packet->Decorate(nulldeco), std::exception) << "Failed to catch null decoration" << std::endl; +} + TEST_F(AutoFilterTest, VerifyInterThreadDecoration) { AutoRequired filterB; AutoRequired factory; From 2875c7683946c5b3811ede9cf2c76218a8fa12f1 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Wed, 16 Jul 2014 15:21:40 -0700 Subject: [PATCH 02/17] Added test for repeated calls when constructing or destroying a packet. --- src/autowiring/test/AutoFilterTest.cpp | 100 +++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index d88be2e6c..31e0d0fb3 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -172,6 +172,106 @@ TEST_F(AutoFilterTest, VerifyNoNullCheckout) { EXPECT_THROW(packet->Decorate(nulldeco), std::exception) << "Failed to catch null decoration" << std::endl; } +template +class FilterGather { +public: + FilterGather(): + m_called_out(0), + m_called_in(0), + m_out(out), + m_in(in) + {} + + void AutoFilter(AutoPacket& packet) { + ++m_called_out; + packet.Decorate(Decoration(m_out)); + } + + void AutoGather(const Decoration& input) { + ++m_called_in; + m_in = input.i; + } + + int m_called_out; + int m_called_in; + int m_out; + int m_in; + NewAutoFilter::AutoGather), &FilterGather::AutoGather> FilterGather_AutoGather; +}; + +TEST_F(AutoFilterTest, VerifyTwoAutoFilterCalls) { + AutoCurrentContext()->Initiate(); + AutoRequired factory; + AutoRequired> zero2one; + AutoRequired> one2zero; + zero2one->m_out = 3; + one2zero->m_out = 4; + + //Verify that calls made on allocation from object pool do not introduce a race condition + { + std::shared_ptr packet = factory->NewPacket(); + ASSERT_EQ(1, zero2one->m_called_out) << "AutoFilter with AutoPacket as only argument was called " << zero2one->m_called_out << " times"; + ASSERT_EQ(1, zero2one->m_called_in) << "AutoFilter of implicitly decorated type was called " << zero2one->m_called_in << " times"; + ASSERT_EQ(4, zero2one->m_in) << "AutoFilter received incorrect input of " << zero2one->m_in; + ASSERT_EQ(1, one2zero->m_called_out) << "AutoFilter with AutoPacket as only argument was called " << one2zero->m_called_out << " times"; + ASSERT_EQ(1, one2zero->m_called_in) << "AutoFilter of implicitly decorated type was called " << one2zero->m_called_in << " times"; + ASSERT_EQ(3, one2zero->m_in) << "AutoFilter received incorrect input of " << one2zero->m_in; + zero2one->m_out = 5; + one2zero->m_out = 6; + } + //Verify that no additional calls are made during return of packet to object pool + ASSERT_EQ(1, zero2one->m_called_out) << "AutoFilter with AutoPacket as only argument was called " << zero2one->m_called_out << " times"; + ASSERT_EQ(1, zero2one->m_called_in) << "AutoFilter of implicitly decorated type was called " << zero2one->m_called_in << " times"; + ASSERT_EQ(4, zero2one->m_in) << "AutoFilter received incorrect input of " << zero2one->m_in; + ASSERT_EQ(1, one2zero->m_called_out) << "AutoFilter with AutoPacket as only argument was called " << one2zero->m_called_out << " times"; + ASSERT_EQ(1, one2zero->m_called_in) << "AutoFilter of implicitly decorated type was called " << one2zero->m_called_in << " times"; + ASSERT_EQ(3, one2zero->m_in) << "AutoFilter received incorrect input of " << one2zero->m_in; +} + +template +class FilterGatherAutoOut { +public: + FilterGatherAutoOut(): + m_called_out(0), + m_called_in(0), + m_out(out), + m_in(in) + {} + + void AutoFilter(auto_out>& output) { + ++m_called_out; + output->i = m_out; + } + + void AutoGather(const Decoration& input) { + ++m_called_in; + m_in = input.i; + } + + int m_called_out; + int m_called_in; + int m_out; + int m_in; + NewAutoFilter::AutoGather), &FilterGather::AutoGather> FilterGather_AutoGather; +}; + + ASSERT_EQ(1, zero2one->m_called_out) << "AutoFilter with auto_out as only argument was called " << zero2one->m_called_out << " times"; + ASSERT_EQ(1, zero2one->m_called_in) << "AutoFilter with auto_out as only argument was called " << zero2one->m_called_in << " times"; + ASSERT_EQ(4, zero2one->m_in) << "AutoFilter received incorrect input of " << zero2one->m_in; + ASSERT_EQ(1, one2zero->m_called_out) << "AutoFilter with auto_out as only argument was called " << one2zero->m_called_out << " times"; + ASSERT_EQ(1, one2zero->m_called_in) << "AutoFilter with auto_out as only argument was called " << one2zero->m_called_in << " times"; + ASSERT_EQ(3, one2zero->m_in) << "AutoFilter received incorrect input of " << one2zero->m_in; + zero2one->m_out = 5; + one2zero->m_out = 6; + } + //Verify that no additional calls are made during return of packet to object pool + ASSERT_EQ(1, zero2one->m_called_out) << "AutoFilter with auto_out as only argument was called " << zero2one->m_called_out << " times"; + ASSERT_EQ(1, zero2one->m_called_in) << "AutoFilter with auto_out as only argument was called " << zero2one->m_called_in << " times"; + ASSERT_EQ(4, zero2one->m_in) << "AutoFilter received incorrect input of " << zero2one->m_in; + ASSERT_EQ(1, one2zero->m_called_out) << "AutoFilter with auto_out as only argument was called " << one2zero->m_called_out << " times"; + ASSERT_EQ(1, one2zero->m_called_in) << "AutoFilter with auto_out as only argument was called " << one2zero->m_called_in << " times"; + ASSERT_EQ(3, one2zero->m_in) << "AutoFilter received incorrect input of " << one2zero->m_in; +} TEST_F(AutoFilterTest, VerifyInterThreadDecoration) { AutoRequired filterB; AutoRequired factory; From bf89fb2d06bf3124a6dd11eefaa59ed0ebfe41f7 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Wed, 16 Jul 2014 17:17:48 -0700 Subject: [PATCH 03/17] Methods with auto_out as only argument are called twice during construction of packet. --- src/autowiring/test/AutoFilterTest.cpp | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index 31e0d0fb3..b8001a13c 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -238,7 +238,8 @@ class FilterGatherAutoOut { m_in(in) {} - void AutoFilter(auto_out>& output) { + //NOTE: Non-const auto_out yields obscure compiler error. + void AutoFilter(const auto_out>& output) { ++m_called_out; output->i = m_out; } @@ -255,23 +256,35 @@ class FilterGatherAutoOut { NewAutoFilter::AutoGather), &FilterGather::AutoGather> FilterGather_AutoGather; }; +TEST_F(AutoFilterTest, VerifyTwoAutoFilterCallsAutoOut) { + AutoCurrentContext()->Initiate(); + AutoRequired factory; + AutoRequired> zero2one; + AutoRequired> one2zero; + zero2one->m_out = 3; + one2zero->m_out = 4; + + //Verify that calls made on allocation from object pool do not introduce a race condition + { + std::shared_ptr packet = factory->NewPacket(); ASSERT_EQ(1, zero2one->m_called_out) << "AutoFilter with auto_out as only argument was called " << zero2one->m_called_out << " times"; - ASSERT_EQ(1, zero2one->m_called_in) << "AutoFilter with auto_out as only argument was called " << zero2one->m_called_in << " times"; + ASSERT_EQ(1, zero2one->m_called_in) << "AutoFilter of implicitly decorated type was called " << zero2one->m_called_in << " times"; ASSERT_EQ(4, zero2one->m_in) << "AutoFilter received incorrect input of " << zero2one->m_in; ASSERT_EQ(1, one2zero->m_called_out) << "AutoFilter with auto_out as only argument was called " << one2zero->m_called_out << " times"; - ASSERT_EQ(1, one2zero->m_called_in) << "AutoFilter with auto_out as only argument was called " << one2zero->m_called_in << " times"; + ASSERT_EQ(1, one2zero->m_called_in) << "AutoFilter of implicitly decorated type was called " << one2zero->m_called_in << " times"; ASSERT_EQ(3, one2zero->m_in) << "AutoFilter received incorrect input of " << one2zero->m_in; zero2one->m_out = 5; one2zero->m_out = 6; } //Verify that no additional calls are made during return of packet to object pool ASSERT_EQ(1, zero2one->m_called_out) << "AutoFilter with auto_out as only argument was called " << zero2one->m_called_out << " times"; - ASSERT_EQ(1, zero2one->m_called_in) << "AutoFilter with auto_out as only argument was called " << zero2one->m_called_in << " times"; + ASSERT_EQ(1, zero2one->m_called_in) << "AutoFilter of implicitly decorated type was called " << zero2one->m_called_in << " times"; ASSERT_EQ(4, zero2one->m_in) << "AutoFilter received incorrect input of " << zero2one->m_in; ASSERT_EQ(1, one2zero->m_called_out) << "AutoFilter with auto_out as only argument was called " << one2zero->m_called_out << " times"; - ASSERT_EQ(1, one2zero->m_called_in) << "AutoFilter with auto_out as only argument was called " << one2zero->m_called_in << " times"; + ASSERT_EQ(1, one2zero->m_called_in) << "AutoFilter of implicitly decorated type was called " << one2zero->m_called_in << " times"; ASSERT_EQ(3, one2zero->m_in) << "AutoFilter received incorrect input of " << one2zero->m_in; } + TEST_F(AutoFilterTest, VerifyInterThreadDecoration) { AutoRequired filterB; AutoRequired factory; From c89c2ef286e0628d844c0da39b0e081ca5df1866 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Wed, 16 Jul 2014 17:19:17 -0700 Subject: [PATCH 04/17] Prevent multiple calls during construction. --- src/autowiring/AutoPacket.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/autowiring/AutoPacket.cpp b/src/autowiring/AutoPacket.cpp index 3b877d6f4..c2e70189d 100644 --- a/src/autowiring/AutoPacket.cpp +++ b/src/autowiring/AutoPacket.cpp @@ -175,10 +175,16 @@ void AutoPacket::Initialize(void) { decoration.second.Reset(); } - // Call all subscribers with no required or optional arguments: + // Find all subscribers with no required or optional arguments: + std::list callCounters; for (auto& satCounter : m_satCounters) if (satCounter) - satCounter.CallAutoFilter(*this); + callCounters.push_back(&satCounter); + + // Call all subscribers with no required or optional arguments: + // NOTE: This may result in decorations that cause other subscribers to be called. + for (auto* call : callCounters) + call->CallAutoFilter(*this); // Initial satisfaction of the AutoPacket: UpdateSatisfaction(typeid(AutoPacket)); From 05e3e51ab92138283c7019d1d127fcbf34626557 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Mon, 28 Jul 2014 17:21:22 -0700 Subject: [PATCH 05/17] Creating separate Initialize and Finalize referenced functions for ObjectPool. This causes some AutoFilter tests to fail. --- autowiring/ObjectPool.h | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/autowiring/ObjectPool.h b/autowiring/ObjectPool.h index 695aff9aa..1f7c322b5 100644 --- a/autowiring/ObjectPool.h +++ b/autowiring/ObjectPool.h @@ -16,7 +16,10 @@ T* DefaultCreate(void) { } template -void DefaultReset(T&){} +void DefaultInitialize(T&){} + +template +void DefaultFinalize(T&){} /// /// Allows the management of a pool of objects based on an embedded factory @@ -43,25 +46,28 @@ class ObjectPool size_t limit = ~0, size_t maxPooled = ~0, const std::function& alloc = &DefaultCreate, - const std::function& rx = &DefaultReset + const std::function& initial = &DefaultInitialize, + const std::function& final = &DefaultFinalize ) : m_monitor(std::make_shared(this)), m_poolVersion(0), m_maxPooled(maxPooled), m_limit(limit), m_outstanding(0), - m_rx(rx), - m_alloc(alloc) + m_alloc(alloc), + m_initial(initial), + m_final(final) {} /// The maximum number of objects this pool will allow to be outstanding at any time ObjectPool( const std::function& alloc, - const std::function& rx = &DefaultReset, + const std::function& initial = &DefaultInitialize, + const std::function& final = &DefaultFinalize, size_t limit = ~0, size_t maxPooled = ~0 ) : - ObjectPool(limit, maxPooled, alloc, rx) + ObjectPool(limit, maxPooled, alloc, initial, final) {} ObjectPool(ObjectPool&& rhs) @@ -95,8 +101,9 @@ class ObjectPool size_t m_limit; size_t m_outstanding; - // Resetter: - std::function m_rx; + // Resetters: + std::function m_initial; + std::function m_final; // Allocator: std::function m_alloc; @@ -144,7 +151,7 @@ class ObjectPool m_objs.size() < m_maxPooled ) { // Reset the object and put it back in the pool: - m_rx(*unique); + m_final(*unique); m_objs.push_back(Wrap(unique.release())); } @@ -170,11 +177,14 @@ class ObjectPool lk.unlock(); // We failed to recover an object, create a new one: - return Wrap(m_alloc()); + auto obj = Wrap(m_alloc()); + m_initial(*obj); + return obj; } // Remove, return: auto obj = m_objs.back(); + m_initial(*obj); m_objs.pop_back(); return obj; } @@ -406,8 +416,9 @@ class ObjectPool m_maxPooled = rhs.m_maxPooled; m_limit = rhs.m_limit; m_outstanding = rhs.m_outstanding; - std::swap(m_rx, rhs.m_rx); std::swap(m_alloc, rhs.m_alloc); + std::swap(m_initial, rhs.m_initial); + std::swap(m_final, rhs.m_final); // Now we can take ownership of this monitor object: m_monitor->SetOwner(this); From eaa554e4cb5b5127ef24cea8a4f6bca81e1e48da Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Mon, 28 Jul 2014 21:10:23 -0700 Subject: [PATCH 06/17] Whitespace twiddles. --- autowiring/AutoFilterDescriptor.h | 2 +- src/autowiring/test/AutoFilterTest.cpp | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/autowiring/AutoFilterDescriptor.h b/autowiring/AutoFilterDescriptor.h index 80e846fdb..38aaa5df9 100644 --- a/autowiring/AutoFilterDescriptor.h +++ b/autowiring/AutoFilterDescriptor.h @@ -448,4 +448,4 @@ namespace std { return (size_t) subscriber.GetAutoFilter()->ptr(); } }; -} \ No newline at end of file +} diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index b8001a13c..6eaff96d8 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -196,6 +196,7 @@ class FilterGather { int m_called_in; int m_out; int m_in; + NewAutoFilter::AutoGather), &FilterGather::AutoGather> FilterGather_AutoGather; }; @@ -219,6 +220,7 @@ TEST_F(AutoFilterTest, VerifyTwoAutoFilterCalls) { zero2one->m_out = 5; one2zero->m_out = 6; } + //Verify that no additional calls are made during return of packet to object pool ASSERT_EQ(1, zero2one->m_called_out) << "AutoFilter with AutoPacket as only argument was called " << zero2one->m_called_out << " times"; ASSERT_EQ(1, zero2one->m_called_in) << "AutoFilter of implicitly decorated type was called " << zero2one->m_called_in << " times"; From 5ccee337c599e412d9192265ae0cc0bd8bb81312 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Mon, 28 Jul 2014 21:11:20 -0700 Subject: [PATCH 07/17] AutoPacket has Reset, Initialize and Finalize methods. Now, all tests pass. --- autowiring/AutoPacket.h | 37 ++++++++++++++++-- src/autowiring/AutoPacket.cpp | 73 +++++++++++++++++------------------ 2 files changed, 69 insertions(+), 41 deletions(-) diff --git a/autowiring/AutoPacket.h b/autowiring/AutoPacket.h index 86a3a34c8..a53544a21 100644 --- a/autowiring/AutoPacket.h +++ b/autowiring/AutoPacket.h @@ -12,7 +12,6 @@ #include STL_UNORDERED_MAP #include EXCEPTION_PTR_HEADER -struct SatCounter; class AutoPacketFactory; class AutoPacketProfiler; struct AutoFilterDescriptor; @@ -58,15 +57,45 @@ class AutoPacket: t_decorationMap m_decorations; /// - /// Last change call with unsatisfied optional arguments + /// Resets satisfaction counters and decoration status. /// - void ResolveOptions(void); + /// + /// Is it expected that AutoPacketFactory will call methods in the following order: + /// AutoPacket(); //Construction in ObjectPool + /// Initialize(); //Issued from ObjectPool + /// Decorate(); + /// ... //More Decorate calls + /// Finalize(); //Returned to ObjectPool + /// Initialize(); + /// ... //More Issue & Return cycles + /// ~AutoPacket(); //Destruction in ObjectPool + /// Reset() must be called before the body of Initialize() in order to begin in the + /// correct state. It must also be called after the body of Finalize() in order to + /// avoid holding shared_ptr references. + /// Therefore Reset() is called at the conclusion of both AutoPacket() and Finalize(). + /// + void Reset(void); /// - /// Resets counters, then decrements subscribers requiring AutoPacket argument. + /// Decrements subscribers requiring AutoPacket argument then calls all initializing subscribers. /// + /// + /// Initialize is called when a packet is issued by the AutoPacketFactory. + /// It is not called when the Packet is created since that could result in + /// spurious calls when no packet is issued. + /// void Initialize(void); + /// + /// Last chance call with unsatisfied optional arguments. + /// + /// + /// This is called when the packet is returned to the AutoPacketFactory. + /// It is not called when the Packet is destroyed, since that could result in + /// suprious calles when no packet is issued. + /// + void Finalize(void); + /// /// Marks the specified entry as being unsatisfiable /// diff --git a/src/autowiring/AutoPacket.cpp b/src/autowiring/AutoPacket.cpp index c2e70189d..3df126b70 100644 --- a/src/autowiring/AutoPacket.cpp +++ b/src/autowiring/AutoPacket.cpp @@ -8,6 +8,8 @@ #include "SatCounter.h" #include +#include + AutoPacket::AutoPacket(AutoPacketFactory& factory) { // Traverse all contexts, adding their packet subscriber vectors one at a time: @@ -52,46 +54,23 @@ AutoPacket::AutoPacket(AutoPacketFactory& factory) } } - // Invoke any output-only AutoFilter routines - Initialize(); + Reset(); } -AutoPacket::~AutoPacket() { - // Last chance for AutoFilter call - ResolveOptions(); -} +// This must appear in .cpp in order to avoid compilation failure due to: +// "Arithmetic on a point to an incomplete type 'SatCounter'" +AutoPacket::~AutoPacket() {} ObjectPool AutoPacket::CreateObjectPool(AutoPacketFactory& factory) { return ObjectPool( ~0, ~0, [&factory] { return new AutoPacket(factory); }, - [] (AutoPacket& packet) { - // Last chance for AutoFilter optional calls - packet.ResolveOptions(); - - // Reinitialize the whole packet: - packet.Initialize(); - } + [] (AutoPacket& packet) { packet.Initialize(); }, + [] (AutoPacket& packet) { packet.Finalize(); } ); } -void AutoPacket::ResolveOptions(void) { - // Queue calls to ensure that calls to Decorate inside of AutoFilter methods - // will NOT effect the resolution of optional arguments. - std::list callQueue; - { - std::lock_guard lk(m_lock); - for(auto& decoration : m_decorations) - for(auto& satCounter : decoration.second.m_subscribers) - if(!satCounter.second) - if(satCounter.first->Resolve()) - callQueue.push_back(satCounter.first); - } - for (SatCounter* call : callQueue) - call->CallAutoFilter(*this); -} - void AutoPacket::MarkUnsatisfiable(const std::type_info& info) { DecorationDisposition* decoration; { @@ -165,16 +144,18 @@ void AutoPacket::PulseSatisfaction(DecorationDisposition* pTypeSubs[], size_t nI } } -void AutoPacket::Initialize(void) { +void AutoPacket::Reset(void) { // Initialize all counters: - { - std::lock_guard lk(m_lock); - for(auto& satCounter : m_satCounters) - satCounter.Reset(); - for(auto& decoration : m_decorations) - decoration.second.Reset(); - } + std::lock_guard lk(m_lock); + for(auto& satCounter : m_satCounters) + satCounter.Reset(); + + // Clear all references: + for(auto& decoration : m_decorations) + decoration.second.Reset(); +} +void AutoPacket::Initialize(void) { // Find all subscribers with no required or optional arguments: std::list callCounters; for (auto& satCounter : m_satCounters) @@ -190,6 +171,24 @@ void AutoPacket::Initialize(void) { UpdateSatisfaction(typeid(AutoPacket)); } +void AutoPacket::Finalize(void) { + // Queue calls to ensure that calls to Decorate inside of AutoFilter methods + // will NOT effect the resolution of optional arguments. + std::list callQueue; + { + std::lock_guard lk(m_lock); + for(auto& decoration : m_decorations) + for(auto& satCounter : decoration.second.m_subscribers) + if(!satCounter.second) + if(satCounter.first->Resolve()) + callQueue.push_back(satCounter.first); + } + for (SatCounter* call : callQueue) + call->CallAutoFilter(*this); + + Reset(); +} + bool AutoPacket::HasSubscribers(const std::type_info& ti) const { return m_decorations.count(ti) != 0; } From da232c493a43498b17d5a1cbaf7e6bd99ca07d24 Mon Sep 17 00:00:00 2001 From: James Donald Date: Fri, 25 Jul 2014 15:14:12 -0700 Subject: [PATCH 08/17] Say what type T is on decoration-already-exists error, #9891 --- autowiring/AutoPacket.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/autowiring/AutoPacket.h b/autowiring/AutoPacket.h index a53544a21..3ad428181 100644 --- a/autowiring/AutoPacket.h +++ b/autowiring/AutoPacket.h @@ -241,8 +241,12 @@ class AutoPacket: { std::lock_guard lk(m_lock); auto& entry = m_decorations[typeid(type)]; - if(entry.satisfied) - throw std::runtime_error("Cannot decorate this packet with type T, the requested decoration already exists"); + if (entry.satisfied) { + std::stringstream ss; + ss << "Cannot decorate this packet with type " << typeid(type).name() + << ", the requested decoration already exists"; + throw std::runtime_error(ss.str()); + } if(entry.isCheckedOut) throw std::runtime_error("Cannot check out this decoration, it's already checked out elsewhere"); entry.isCheckedOut = true; From 104c6c0c9aa2772939f22a95b612ef93141b349e Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Fri, 25 Jul 2014 16:17:39 -0700 Subject: [PATCH 09/17] Make all type repetition errors explicit about type in AutoPacket. In order to see the output of an error, just remove the corresponding EXPECT_ANY_THROW enclosure in TEST_F(AutoFilterTest, VerifyNoMultiDecorate). --- autowiring/AutoPacket.h | 28 +++++++++++++++++++------- src/autowiring/test/AutoFilterTest.cpp | 18 +++++++++++++++++ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/autowiring/AutoPacket.h b/autowiring/AutoPacket.h index 3ad428181..94ceeaf13 100644 --- a/autowiring/AutoPacket.h +++ b/autowiring/AutoPacket.h @@ -7,6 +7,8 @@ #include "is_shared_ptr.h" #include "ObjectPool.h" #include "is_any.h" +#include +#include #include MEMORY_HEADER #include TYPE_INDEX_HEADER #include STL_UNORDERED_MAP @@ -167,8 +169,12 @@ class AutoPacket: template const T& Get(void) const { const T* retVal; - if(!Get(retVal)) - throw_rethrowable autowiring_error("Attempted to obtain a value which was not decorated on this packet"); + if(!Get(retVal)) { + std::stringstream ss; + ss << "Attempted to obtain a type " << typeid(retVal).name() + << " which was not decorated on this packet"; + throw std::runtime_error(ss.str()); + } return *retVal; } @@ -243,12 +249,16 @@ class AutoPacket: auto& entry = m_decorations[typeid(type)]; if (entry.satisfied) { std::stringstream ss; - ss << "Cannot decorate this packet with type " << typeid(type).name() + ss << "Cannot decorate this packet with type " << typeid(*ptr).name() << ", the requested decoration already exists"; throw std::runtime_error(ss.str()); } - if(entry.isCheckedOut) - throw std::runtime_error("Cannot check out this decoration, it's already checked out elsewhere"); + if(entry.isCheckedOut) { + std::stringstream ss; + ss << "Cannot check out decoration of type " << typeid(*ptr).name() + << ", it is already checked out elsewhere"; + throw std::runtime_error(ss.str()); + } entry.isCheckedOut = true; entry.wasCheckedOut = true; } @@ -353,8 +363,12 @@ class AutoPacket: std::lock_guard lk(m_lock); for(size_t i = 0; i < sizeof...(Ts); i++) { pTypeSubs[i] = &m_decorations[*sc_typeInfo[i]]; - if(pTypeSubs[i]->wasCheckedOut) - throw std::runtime_error("Cannot perform immediate decoration with type T, the requested decoration already exists"); + if(pTypeSubs[i]->wasCheckedOut) { + std::stringstream ss; + ss << "Cannot perform immediate decoration with type " << sc_typeInfo[i]->name() + << ", the requested decoration already exists"; + throw std::runtime_error(ss.str()); + } // Mark the entry as appropriate: pTypeSubs[i]->satisfied = true; diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index 6eaff96d8..8b2e72673 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -158,6 +158,24 @@ TEST_F(AutoFilterTest, VerifyNoMultiDecorate) { // Now finish saturating the filter and ensure we get a call: packet->Decorate(Decoration<1>()); EXPECT_LT(0, filterA->m_called) << "Filter was not called after being fully satisfied"; + + // Verify that DecorateImmedaite also yields an exception + Decoration<0> localDeco0; + EXPECT_ANY_THROW(packet->DecorateImmediate(localDeco0)) << "Redundant immediate decoration did not throw an exception as expected"; + + //NOTE: A typedef will throw an exception + typedef Decoration<0> isDeco0type; + EXPECT_ANY_THROW(packet->DecorateImmediate(isDeco0type())) << "Typedef failed to throw exception"; + + //NOTE: Inheritance will not throw an exception + class ofDeco0alias: public Decoration<0> {}; + try { + packet->Decorate(ofDeco0alias()); + } catch (...) { + FAIL() << "Class with inheritance reinterpreted as child type"; + } + + EXPECT_ANY_THROW(packet->DecorateImmediate(Decoration<2>(), Decoration<2>())) << "Repeated type in immediate decoration was not identified as an error"; } TEST_F(AutoFilterTest, VerifyNoNullCheckout) { From bb804e231f0a8cc6281c69559cf8dcb9a6dc501b Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Fri, 25 Jul 2014 17:16:06 -0700 Subject: [PATCH 10/17] Adding test for shared_ptr reduction to base-type for type redecoration. --- src/autowiring/test/AutoFilterTest.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index 8b2e72673..8064afc87 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -159,22 +159,26 @@ TEST_F(AutoFilterTest, VerifyNoMultiDecorate) { packet->Decorate(Decoration<1>()); EXPECT_LT(0, filterA->m_called) << "Filter was not called after being fully satisfied"; - // Verify that DecorateImmedaite also yields an exception - Decoration<0> localDeco0; - EXPECT_ANY_THROW(packet->DecorateImmediate(localDeco0)) << "Redundant immediate decoration did not throw an exception as expected"; - //NOTE: A typedef will throw an exception typedef Decoration<0> isDeco0type; - EXPECT_ANY_THROW(packet->DecorateImmediate(isDeco0type())) << "Typedef failed to throw exception"; + EXPECT_ANY_THROW(packet->Decorate(isDeco0type())) << "Typedef failed to throw exception"; + + //NOTE: A shared_ptr to an existing type will throw an exception + std::shared_ptr> sharedDeco0(new Decoration<0>); + EXPECT_ANY_THROW(packet->Decorate(sharedDeco0)) << "Reduction of shared_ptr to base type failed"; //NOTE: Inheritance will not throw an exception class ofDeco0alias: public Decoration<0> {}; try { packet->Decorate(ofDeco0alias()); } catch (...) { - FAIL() << "Class with inheritance reinterpreted as child type"; + FAIL() << "Class with inheritance from existing decoration reinterpreted as child type"; } + // Verify that DecorateImmedaite also yields an exception + Decoration<0> localDeco0; + EXPECT_ANY_THROW(packet->DecorateImmediate(localDeco0)) << "Redundant immediate decoration did not throw an exception as expected"; + EXPECT_ANY_THROW(packet->DecorateImmediate(Decoration<2>(), Decoration<2>())) << "Repeated type in immediate decoration was not identified as an error"; } From 441a2f9ed09b6ebef78bb136b716c04d4a7d4b13 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Tue, 29 Jul 2014 17:07:12 -0700 Subject: [PATCH 11/17] Adding life-cycle testing to AutoPacket. Presently, the lifecycle assumptions are not satisfied. --- autowiring/AutoPacket.h | 34 ++++++++++++++++++++++++++++++++++ src/autowiring/AutoPacket.cpp | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/autowiring/AutoPacket.h b/autowiring/AutoPacket.h index 94ceeaf13..1156227e7 100644 --- a/autowiring/AutoPacket.h +++ b/autowiring/AutoPacket.h @@ -40,6 +40,16 @@ class AutoPacket: AutoPacket(const AutoPacket& rhs) = delete; AutoPacket(AutoPacketFactory& factory); + //DEBUG BEGIN: Verify adherence to life plans + //QUESTION: Does this need to be promoted to a condition + //for calling Finalize in the destructor? + int debug_lifecycle; + static const int lifecycle_construct = 0; + static const int lifecycle_inpool = 1; + static const int lifecycle_issued = 2; + static const int lifecycle_destruct = 3; + //DEBUG END + public: ~AutoPacket(void); @@ -237,6 +247,12 @@ class AutoPacket: /// template AutoCheckout Checkout(std::shared_ptr ptr) { + if (!(debug_lifecycle == lifecycle_issued)) { + std::stringstream ss; + ss << "Checkout called in lifecycle: " << debug_lifecycle; + //throw std::runtime_error(ss.str()); + } + // This allows us to install correct entries for decorated input requests typedef typename subscriber_traits::type type; @@ -290,6 +306,12 @@ class AutoPacket: /// template void Unsatisfiable(void) { + if (!(debug_lifecycle == lifecycle_issued)) { + std::stringstream ss; + ss << "Unsatisfiable called in lifecycle: " << debug_lifecycle; + //throw std::runtime_error(ss.str()); + } + { // Insert a null entry at this location: std::lock_guard lk(m_lock); @@ -328,6 +350,12 @@ class AutoPacket: /// template const T& Decorate(std::shared_ptr t) { + if (!(debug_lifecycle == lifecycle_issued)) { + std::stringstream ss; + ss << "Decorate called in lifecycle: " << debug_lifecycle; + //throw std::runtime_error(ss.str()); + } + Checkout(t).Ready(); return *t; } @@ -346,6 +374,12 @@ class AutoPacket: /// template void DecorateImmediate(const Ts&... immeds) { + if (!(debug_lifecycle == lifecycle_issued)) { + std::stringstream ss; + ss << "DecorateImmediate called in lifecycle: " << debug_lifecycle; + //throw std::runtime_error(ss.str()); + } + // These are the things we're going to be working with while we perform immediate decoration: static const std::type_info* sc_typeInfo [] = {&typeid(Ts)...}; const void* pvImmeds[] = {&immeds...}; diff --git a/src/autowiring/AutoPacket.cpp b/src/autowiring/AutoPacket.cpp index 3df126b70..303eb1779 100644 --- a/src/autowiring/AutoPacket.cpp +++ b/src/autowiring/AutoPacket.cpp @@ -10,7 +10,7 @@ #include -AutoPacket::AutoPacket(AutoPacketFactory& factory) +AutoPacket::AutoPacket(AutoPacketFactory& factory) : debug_lifecycle(0) { // Traverse all contexts, adding their packet subscriber vectors one at a time: for(const auto& curContext : ContextEnumerator(factory.GetContext())) { @@ -59,7 +59,15 @@ AutoPacket::AutoPacket(AutoPacketFactory& factory) // This must appear in .cpp in order to avoid compilation failure due to: // "Arithmetic on a point to an incomplete type 'SatCounter'" -AutoPacket::~AutoPacket() {} +AutoPacket::~AutoPacket() { + if (debug_lifecycle == lifecycle_issued) { + std::cout << "Destruct before Finalize!" << std::endl; + } + if (debug_lifecycle == lifecycle_destruct) { + throw std::runtime_error("Destruct called Twice!"); + } + debug_lifecycle = 3; +} ObjectPool AutoPacket::CreateObjectPool(AutoPacketFactory& factory) { return ObjectPool( @@ -145,6 +153,13 @@ void AutoPacket::PulseSatisfaction(DecorationDisposition* pTypeSubs[], size_t nI } void AutoPacket::Reset(void) { + if (!(debug_lifecycle == lifecycle_construct || debug_lifecycle == lifecycle_issued)) { + std::stringstream ss; + ss << "Reset called in lifecycle: " << debug_lifecycle; + //throw std::runtime_error(ss.str()); + } + debug_lifecycle = lifecycle_inpool; + // Initialize all counters: std::lock_guard lk(m_lock); for(auto& satCounter : m_satCounters) @@ -156,6 +171,13 @@ void AutoPacket::Reset(void) { } void AutoPacket::Initialize(void) { + if (!(debug_lifecycle == lifecycle_inpool)) { + std::stringstream ss; + ss << "Initialize called in lifecycle: " << debug_lifecycle; + //throw std::runtime_error(ss.str()); + } + debug_lifecycle = lifecycle_issued; + // Find all subscribers with no required or optional arguments: std::list callCounters; for (auto& satCounter : m_satCounters) @@ -172,6 +194,12 @@ void AutoPacket::Initialize(void) { } void AutoPacket::Finalize(void) { + if (!(debug_lifecycle == lifecycle_issued)) { + std::stringstream ss; + ss << "Finalize called in lifecycle: " << debug_lifecycle; + //throw std::runtime_error(ss.str()); + } + // Queue calls to ensure that calls to Decorate inside of AutoFilter methods // will NOT effect the resolution of optional arguments. std::list callQueue; From 6192549e89b0e883e5cc99575d155735ac44e539 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Tue, 29 Jul 2014 17:13:58 -0700 Subject: [PATCH 12/17] Restructured ObjectPool to satisfy AutoPacket (or any other object) lifecycle contract. Presently, AutoFilterTest hangs. - The first call on any issued AutoPacket is Initialize() and last call is Finalize(). - The internal store of objects is now a vector of unique_ptr objects. This avoids calling m_final incorrectly and allows the release() method to be used when issuing objects. - Move assignment between object pools now swaps pools instead of copying. --- autowiring/ObjectPool.h | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/autowiring/ObjectPool.h b/autowiring/ObjectPool.h index 1f7c322b5..d8f6dfe14 100644 --- a/autowiring/ObjectPool.h +++ b/autowiring/ObjectPool.h @@ -95,7 +95,7 @@ class ObjectPool // time the ClearCachedEntities method is called, and causes entities which might be trying // to return to the pool to instead free themselves. size_t m_poolVersion; - std::vector> m_objs; + std::vector> m_objs; size_t m_maxPooled; size_t m_limit; @@ -109,18 +109,29 @@ class ObjectPool std::function m_alloc; /// - /// Creates a shared pointer to wrap the specified object + /// Creates a shared pointer to wrap the specified object while it is issued /// + /// + /// The Initialize is applied immediate when Wrap is called. + /// The Finalize function will be applied is in the shared_ptr destructor. + /// std::shared_ptr Wrap(T* pObj) { + // Initialize the issued object + m_initial(*pObj); + // Fill the shared pointer with the object we created, and ensure that we override // the destructor so that the object is returned to the pool when it falls out of // scope. size_t poolVersion = m_poolVersion; auto monitor = m_monitor; + std::function final = m_final; return std::shared_ptr( pObj, - [poolVersion, monitor](T* ptr) { + [poolVersion, monitor, final](T* ptr) { + // Finalize object before destruction or return to pool + final(*ptr); + // Default behavior will be to destroy the pointer std::unique_ptr unique(ptr); @@ -139,6 +150,7 @@ class ObjectPool } void Return(size_t poolVersion, std::unique_ptr& unique) { + // ASSERT: Object has already been finalized // Always decrement the count when an object is no longer outstanding assert(m_outstanding); m_outstanding--; @@ -150,9 +162,8 @@ class ObjectPool // Object pool needs to be capable of accepting another object as an input m_objs.size() < m_maxPooled ) { - // Reset the object and put it back in the pool: - m_final(*unique); - m_objs.push_back(Wrap(unique.release())); + // Return the object to the pool: + m_objs.emplace_back(std::move(unique)); } // If the new outstanding count is less than or equal to the limit, wake up any waiters: @@ -178,15 +189,13 @@ class ObjectPool // We failed to recover an object, create a new one: auto obj = Wrap(m_alloc()); - m_initial(*obj); return obj; } - // Remove, return: - auto obj = m_objs.back(); - m_initial(*obj); - m_objs.pop_back(); - return obj; + // Transition from pooled to issued: + std::shared_ptr iObj = Wrap(m_objs.back().release()); // Takes ownership + m_objs.pop_back(); // Removes non-referencing object + return iObj; } public: @@ -217,7 +226,7 @@ class ObjectPool /// void ClearCachedEntities(void) { // Declare this first, so it's freed last: - std::vector> objs; + std::vector> objs; // Move all of our objects into a local variable which we can then free at our leisure. This allows us to // perform destruction outside of the scope of a lock, preventing any deadlocks that might occur inside @@ -243,7 +252,7 @@ class ObjectPool void SetMaximumPooledEntities(size_t maxPooled) { m_maxPooled = maxPooled; for(;;) { - std::shared_ptr prior; + std::unique_ptr prior; std::lock_guard lk(*m_monitor); // Space check: @@ -256,7 +265,7 @@ class ObjectPool // Funny syntax needed to ensure destructors run while we aren't holding any locks. The prior // shared_ptr will be reset after the lock is released, guaranteeing the desired ordering. - prior = m_objs.back(); + prior = std::move(m_objs.back()); m_objs.pop_back(); } } @@ -412,10 +421,10 @@ class ObjectPool std::swap(m_monitor, rhs.m_monitor); m_poolVersion = rhs.m_poolVersion; - m_objs = rhs.m_objs; m_maxPooled = rhs.m_maxPooled; m_limit = rhs.m_limit; m_outstanding = rhs.m_outstanding; + std::swap(m_objs, rhs.m_objs); std::swap(m_alloc, rhs.m_alloc); std::swap(m_initial, rhs.m_initial); std::swap(m_final, rhs.m_final); From 8bcdd72a3f7090f07cb74d0ec145b29fd8776513 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Tue, 29 Jul 2014 18:56:41 -0700 Subject: [PATCH 13/17] Adding AutoPacketFactory::GetOutstanding in order to test correct outstanding counts. --- autowiring/AutoPacketFactory.h | 3 +++ src/autowiring/test/AutoFilterTest.cpp | 12 ++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/autowiring/AutoPacketFactory.h b/autowiring/AutoPacketFactory.h index d2044d805..5b14d9b4e 100644 --- a/autowiring/AutoPacketFactory.h +++ b/autowiring/AutoPacketFactory.h @@ -119,4 +119,7 @@ class AutoPacketFactory: /// satisfaction graph /// std::shared_ptr NewPacket(void); + + /// the number of outstanding AutoPackets + size_t GetOutstanding(void) const { return m_packets.GetOutstanding(); } }; diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index 8064afc87..7ed4d90b3 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -112,8 +112,8 @@ TEST_F(AutoFilterTest, VerifyOptionalFilter) { packet->Decorate(Decoration<0>()); ASSERT_TRUE(fgA->m_called == 0) << "An AutoFilter was called " << fgA->m_called << " times when an optional input was unresolved"; - ASSERT_TRUE(fgB->m_called == 0) << "An AutoFilter was called " << fgA->m_called << " times when an optional input was unresolved"; - ASSERT_TRUE(fgC->m_called == 0) << "An AutoFilter was called " << fgA->m_called << " times when a required input was not available"; + ASSERT_TRUE(fgB->m_called == 0) << "An AutoFilter was called " << fgB->m_called << " times when an optional input was unresolved"; + ASSERT_TRUE(fgC->m_called == 0) << "An AutoFilter was called " << fgC->m_called << " times when a required input was not available"; packet->Decorate(Decoration<1>()); @@ -133,6 +133,11 @@ TEST_F(AutoFilterTest, VerifyOptionalFilter) { auto packet = factory->NewPacket(); packet->Decorate(Decoration<0>()); packet->Decorate(Decoration<2>()); + + ASSERT_TRUE(fgA->m_called == 0) << "An AutoFilter was called " << fgA->m_called << " before optional arguments were resolved"; + ASSERT_TRUE(fgB->m_called == 0) << "An AutoFilter was called " << fgB->m_called << " before optional arguments were resolved"; + ASSERT_TRUE(fgC->m_called == 0) << "An AutoFilter was called " << fgC->m_called << " before optional arguments were resolved"; + ASSERT_TRUE(fgD->m_called == 0) << "An AutoFilter was called " << fgD->m_called << " before optional arguments were resolved"; } ASSERT_TRUE(fgA->m_called == 1) << "An AutoFilter was called " << fgA->m_called << " times when all required inputs were available"; @@ -699,9 +704,11 @@ TEST_F(AutoFilterTest, SingleImmediate) { // Verify we can't decorate this value a second time: ASSERT_ANY_THROW(packet->DecorateImmediate(val)) << "Expected an exception when a second attempt was made to attach a decoration"; } + ASSERT_EQ(0, factory->GetOutstanding()) << "Destroyed packet remains outstanding"; static const int pattern = 1365; //1365 ~ 10101010101 AutoRequired>> fgp; + ASSERT_EQ(0, factory->GetOutstanding()) << "Outstanding packet count is correct after incrementing m_poolVersion due to AutoFilter addition"; { auto packet = factory->NewPacket(); Decoration dec; @@ -710,6 +717,7 @@ TEST_F(AutoFilterTest, SingleImmediate) { ASSERT_TRUE(fgp->m_called == 1) << "Filter should called " << fgp->m_called << " times, expected 1"; ASSERT_TRUE(std::get<0>(fgp->m_args).i == pattern) << "Filter argument yielded " << std::get<0>(fgp->m_args).i << "expected " << pattern; } + ASSERT_EQ(0, factory->GetOutstanding()) << "Destroyed packet remains outstanding"; // Terminate enclosing context AutoCurrentContext()->SignalShutdown(true); From 08f76f847061f7764fdcdc83e1dade523626c783 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Tue, 29 Jul 2014 19:00:56 -0700 Subject: [PATCH 14/17] Fixed incorrect count of issued packets in ClearCachedEntities(). - This addressed the problem in which the AutowiredTest suite would halt at AutoFilterTest::SingleImmediate. - The AutowiredTest suite now halts at ObjectPoolTest::CanRundownOneIssued. - The count was correct when destruction of objects in the pool would trigger an decrement of m_outstanding. - The incorrect count was triggered because adding an AutoFilter to a context required flushing the pool of packets. - The pool flush seems to have been required because the destructor introduced in Wrap would have an incorrect version number. - The use of Wrapped packets in the pool is because std::unique_ptr is not available in all build configurations. --- autowiring/ObjectPool.h | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/autowiring/ObjectPool.h b/autowiring/ObjectPool.h index d8f6dfe14..1965d57f3 100644 --- a/autowiring/ObjectPool.h +++ b/autowiring/ObjectPool.h @@ -228,17 +228,11 @@ class ObjectPool // Declare this first, so it's freed last: std::vector> objs; - // Move all of our objects into a local variable which we can then free at our leisure. This allows us to - // perform destruction outside of the scope of a lock, preventing any deadlocks that might occur inside - // the shared_ptr cleanup lambda. + // Default destructor is using in object pool, so it is safe to destroy all + // in pool while holding lock. std::lock_guard lk(*m_monitor); m_poolVersion++; - - // Swap the cached object collection with an empty one - std::swap(objs, m_objs); - - // Technically, all of these entities are now outstanding. Update accordingly. - m_outstanding += objs.size(); + m_objs.clear(); } /// From a5318131a9bb89e4f1efec97ecc98602e9099c77 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Tue, 29 Jul 2014 19:50:27 -0700 Subject: [PATCH 15/17] Fixed incorrect count of issued packets in SetMaximumPooledEntities(). --- autowiring/ObjectPool.h | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/autowiring/ObjectPool.h b/autowiring/ObjectPool.h index 1965d57f3..78af0e788 100644 --- a/autowiring/ObjectPool.h +++ b/autowiring/ObjectPool.h @@ -246,7 +246,6 @@ class ObjectPool void SetMaximumPooledEntities(size_t maxPooled) { m_maxPooled = maxPooled; for(;;) { - std::unique_ptr prior; std::lock_guard lk(*m_monitor); // Space check: @@ -254,12 +253,7 @@ class ObjectPool // Managed to get the size down sufficiently, we can continue: return; - // Removing an entry from the cache, must increase the outstanding count at this point - m_outstanding++; - - // Funny syntax needed to ensure destructors run while we aren't holding any locks. The prior - // shared_ptr will be reset after the lock is released, guaranteeing the desired ordering. - prior = std::move(m_objs.back()); + // Remove unique pointer m_objs.pop_back(); } } From d513f3cd81d4f514aa25bcf7186eaeed7f064832 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Tue, 29 Jul 2014 20:04:07 -0700 Subject: [PATCH 16/17] Removing life-cycle consistency checks from AutoPacket. --- autowiring/AutoPacket.h | 34 ---------------------------------- src/autowiring/AutoPacket.cpp | 33 ++------------------------------- 2 files changed, 2 insertions(+), 65 deletions(-) diff --git a/autowiring/AutoPacket.h b/autowiring/AutoPacket.h index 1156227e7..94ceeaf13 100644 --- a/autowiring/AutoPacket.h +++ b/autowiring/AutoPacket.h @@ -40,16 +40,6 @@ class AutoPacket: AutoPacket(const AutoPacket& rhs) = delete; AutoPacket(AutoPacketFactory& factory); - //DEBUG BEGIN: Verify adherence to life plans - //QUESTION: Does this need to be promoted to a condition - //for calling Finalize in the destructor? - int debug_lifecycle; - static const int lifecycle_construct = 0; - static const int lifecycle_inpool = 1; - static const int lifecycle_issued = 2; - static const int lifecycle_destruct = 3; - //DEBUG END - public: ~AutoPacket(void); @@ -247,12 +237,6 @@ class AutoPacket: /// template AutoCheckout Checkout(std::shared_ptr ptr) { - if (!(debug_lifecycle == lifecycle_issued)) { - std::stringstream ss; - ss << "Checkout called in lifecycle: " << debug_lifecycle; - //throw std::runtime_error(ss.str()); - } - // This allows us to install correct entries for decorated input requests typedef typename subscriber_traits::type type; @@ -306,12 +290,6 @@ class AutoPacket: /// template void Unsatisfiable(void) { - if (!(debug_lifecycle == lifecycle_issued)) { - std::stringstream ss; - ss << "Unsatisfiable called in lifecycle: " << debug_lifecycle; - //throw std::runtime_error(ss.str()); - } - { // Insert a null entry at this location: std::lock_guard lk(m_lock); @@ -350,12 +328,6 @@ class AutoPacket: /// template const T& Decorate(std::shared_ptr t) { - if (!(debug_lifecycle == lifecycle_issued)) { - std::stringstream ss; - ss << "Decorate called in lifecycle: " << debug_lifecycle; - //throw std::runtime_error(ss.str()); - } - Checkout(t).Ready(); return *t; } @@ -374,12 +346,6 @@ class AutoPacket: /// template void DecorateImmediate(const Ts&... immeds) { - if (!(debug_lifecycle == lifecycle_issued)) { - std::stringstream ss; - ss << "DecorateImmediate called in lifecycle: " << debug_lifecycle; - //throw std::runtime_error(ss.str()); - } - // These are the things we're going to be working with while we perform immediate decoration: static const std::type_info* sc_typeInfo [] = {&typeid(Ts)...}; const void* pvImmeds[] = {&immeds...}; diff --git a/src/autowiring/AutoPacket.cpp b/src/autowiring/AutoPacket.cpp index 303eb1779..d868fcbdd 100644 --- a/src/autowiring/AutoPacket.cpp +++ b/src/autowiring/AutoPacket.cpp @@ -10,8 +10,7 @@ #include -AutoPacket::AutoPacket(AutoPacketFactory& factory) : debug_lifecycle(0) -{ +AutoPacket::AutoPacket(AutoPacketFactory& factory) { // Traverse all contexts, adding their packet subscriber vectors one at a time: for(const auto& curContext : ContextEnumerator(factory.GetContext())) { AutowiredFast curFactory(curContext); @@ -59,15 +58,7 @@ AutoPacket::AutoPacket(AutoPacketFactory& factory) : debug_lifecycle(0) // This must appear in .cpp in order to avoid compilation failure due to: // "Arithmetic on a point to an incomplete type 'SatCounter'" -AutoPacket::~AutoPacket() { - if (debug_lifecycle == lifecycle_issued) { - std::cout << "Destruct before Finalize!" << std::endl; - } - if (debug_lifecycle == lifecycle_destruct) { - throw std::runtime_error("Destruct called Twice!"); - } - debug_lifecycle = 3; -} +AutoPacket::~AutoPacket() {} ObjectPool AutoPacket::CreateObjectPool(AutoPacketFactory& factory) { return ObjectPool( @@ -153,13 +144,6 @@ void AutoPacket::PulseSatisfaction(DecorationDisposition* pTypeSubs[], size_t nI } void AutoPacket::Reset(void) { - if (!(debug_lifecycle == lifecycle_construct || debug_lifecycle == lifecycle_issued)) { - std::stringstream ss; - ss << "Reset called in lifecycle: " << debug_lifecycle; - //throw std::runtime_error(ss.str()); - } - debug_lifecycle = lifecycle_inpool; - // Initialize all counters: std::lock_guard lk(m_lock); for(auto& satCounter : m_satCounters) @@ -171,13 +155,6 @@ void AutoPacket::Reset(void) { } void AutoPacket::Initialize(void) { - if (!(debug_lifecycle == lifecycle_inpool)) { - std::stringstream ss; - ss << "Initialize called in lifecycle: " << debug_lifecycle; - //throw std::runtime_error(ss.str()); - } - debug_lifecycle = lifecycle_issued; - // Find all subscribers with no required or optional arguments: std::list callCounters; for (auto& satCounter : m_satCounters) @@ -194,12 +171,6 @@ void AutoPacket::Initialize(void) { } void AutoPacket::Finalize(void) { - if (!(debug_lifecycle == lifecycle_issued)) { - std::stringstream ss; - ss << "Finalize called in lifecycle: " << debug_lifecycle; - //throw std::runtime_error(ss.str()); - } - // Queue calls to ensure that calls to Decorate inside of AutoFilter methods // will NOT effect the resolution of optional arguments. std::list callQueue; From 9ca76a65d2dbfeced1f984e5cefbd3f9e7a1cb11 Mon Sep 17 00:00:00 2001 From: Jason Lokerson Date: Wed, 30 Jul 2014 09:52:10 -0700 Subject: [PATCH 17/17] Adding an outstanding count reference to AutoPacket --- autowiring/AutoPacket.h | 8 ++++++-- src/autowiring/AutoPacket.cpp | 13 ++++++++++--- src/autowiring/AutoPacketFactory.cpp | 2 +- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/autowiring/AutoPacket.h b/autowiring/AutoPacket.h index 94ceeaf13..ecd0e4d2a 100644 --- a/autowiring/AutoPacket.h +++ b/autowiring/AutoPacket.h @@ -38,12 +38,12 @@ class AutoPacket: { private: AutoPacket(const AutoPacket& rhs) = delete; - AutoPacket(AutoPacketFactory& factory); + AutoPacket(AutoPacketFactory& factory, const std::shared_ptr& outstanding); public: ~AutoPacket(void); - static ObjectPool CreateObjectPool(AutoPacketFactory& factory); + static ObjectPool CreateObjectPool(AutoPacketFactory& factory, const std::shared_ptr& outstanding); private: // A back-link to the previously issued packet in the packet sequence. May potentially be null, @@ -58,6 +58,10 @@ class AutoPacket: typedef std::unordered_map t_decorationMap; t_decorationMap m_decorations; + // Outstanding count local and remote holds: + std::shared_ptr m_outstanding; + const std::shared_ptr& m_outstandingRemote; + /// /// Resets satisfaction counters and decoration status. /// diff --git a/src/autowiring/AutoPacket.cpp b/src/autowiring/AutoPacket.cpp index d868fcbdd..b018a1f42 100644 --- a/src/autowiring/AutoPacket.cpp +++ b/src/autowiring/AutoPacket.cpp @@ -10,7 +10,9 @@ #include -AutoPacket::AutoPacket(AutoPacketFactory& factory) { +AutoPacket::AutoPacket(AutoPacketFactory& factory, const std::shared_ptr& outstanding): + m_outstandingRemote(outstanding) +{ // Traverse all contexts, adding their packet subscriber vectors one at a time: for(const auto& curContext : ContextEnumerator(factory.GetContext())) { AutowiredFast curFactory(curContext); @@ -60,11 +62,11 @@ AutoPacket::AutoPacket(AutoPacketFactory& factory) { // "Arithmetic on a point to an incomplete type 'SatCounter'" AutoPacket::~AutoPacket() {} -ObjectPool AutoPacket::CreateObjectPool(AutoPacketFactory& factory) { +ObjectPool AutoPacket::CreateObjectPool(AutoPacketFactory& factory, const std::shared_ptr& outstanding) { return ObjectPool( ~0, ~0, - [&factory] { return new AutoPacket(factory); }, + [&factory, &outstanding] { return new AutoPacket(factory, outstanding); }, [] (AutoPacket& packet) { packet.Initialize(); }, [] (AutoPacket& packet) { packet.Finalize(); } ); @@ -155,6 +157,11 @@ void AutoPacket::Reset(void) { } void AutoPacket::Initialize(void) { + // Hold an outstanding count from the parent packet factory + m_outstanding = m_outstandingRemote; + if(!m_outstanding) + throw autowiring_error("Cannot proceed with this packet, enclosing context already expired"); + // Find all subscribers with no required or optional arguments: std::list callCounters; for (auto& satCounter : m_satCounters) diff --git a/src/autowiring/AutoPacketFactory.cpp b/src/autowiring/AutoPacketFactory.cpp index 7b5cb0e30..13fb1312c 100644 --- a/src/autowiring/AutoPacketFactory.cpp +++ b/src/autowiring/AutoPacketFactory.cpp @@ -10,7 +10,7 @@ AutoPacketFactory::AutoPacketFactory(void): ContextMember("AutoPacketFactory"), m_parent(GetContext()->GetParentContext()), m_wasStopped(false), - m_packets(AutoPacket::CreateObjectPool(*this)) + m_packets(AutoPacket::CreateObjectPool(*this, m_outstanding)) {} AutoPacketFactory::~AutoPacketFactory() {