From d8695c3fc22f3815340a27e32c4755e707dd71a6 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Tue, 17 Feb 2026 13:16:43 +0000 Subject: [PATCH 1/3] Canceller : Derive from RefCounted This will be useful in Gaffer, where there are a couple of spots where we must pass stack-allocated Cancellers to Python as raw pointers, and just hope that nobody accesses them after they expire. It also enables the next commit, where we allow cancellers to be chained. --- Changes | 10 ++++++++-- include/IECore/Canceller.h | 12 ++++++------ src/IECorePython/CancellerBinding.cpp | 6 +++--- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/Changes b/Changes index d95afdf26f..6f5d39cdc0 100644 --- a/Changes +++ b/Changes @@ -1,12 +1,18 @@ 10.7.x.x (relative to 10.7.0.0a4) ======== -API ---- +Improvements +------------ +- Canceller : Derived from RefCounted, to allow Cancellers to be shared between multiple clients. - MessageHandler : Added `msg()` overload that accepts a `fmt::format_string` and arguments. - InternedString : Added specialisation for `fmt::formatter`. +Breaking Changes +---------------- + +- Canceller : Changed base class. + Build ----- diff --git a/include/IECore/Canceller.h b/include/IECore/Canceller.h index 8d53977c73..44b9438656 100644 --- a/include/IECore/Canceller.h +++ b/include/IECore/Canceller.h @@ -35,9 +35,7 @@ #ifndef IECORE_CANCELLER_H #define IECORE_CANCELLER_H -#include "IECore/Export.h" - -#include "boost/noncopyable.hpp" +#include "IECore/RefCounted.h" #include #include @@ -61,18 +59,18 @@ struct IECORE_API Cancelled /// Example : /// /// ``` -/// Canceller c; +/// CancellerPtr c = new Canceller; /// thread t( /// [&c] { /// while( 1 ) { -/// Canceller::check( &c ); +/// Canceller::check( c.get() ); /// } /// } /// ); /// c.cancel(); /// t.join(); /// ``` -class Canceller : public boost::noncopyable +class IECORE_API Canceller : public IECore::RefCounted { public : @@ -82,6 +80,8 @@ class Canceller : public boost::noncopyable { } + IE_CORE_DECLAREMEMBERPTR( Canceller ) + void cancel() { // Store time of first cancellation. We use `compare_exchange_weak()` diff --git a/src/IECorePython/CancellerBinding.cpp b/src/IECorePython/CancellerBinding.cpp index bff05d83eb..5b8bb19a8c 100644 --- a/src/IECorePython/CancellerBinding.cpp +++ b/src/IECorePython/CancellerBinding.cpp @@ -38,6 +38,7 @@ #include "IECorePython/CancellerBinding.h" #include "IECorePython/ExceptionBinding.h" +#include "IECorePython/RefCountedBinding.h" #include "IECore/Canceller.h" @@ -60,7 +61,8 @@ namespace IECorePython void bindCanceller() { - class_( "Canceller" ) + RefCountedClass( "Canceller" ) + .def( init<>() ) .def( "cancel", &Canceller::cancel ) .def( "cancelled", &Canceller::cancelled ) .def( "check", &Canceller::check ) @@ -68,8 +70,6 @@ void bindCanceller() .def( "elapsedTime", &elapsedTimeWrapper ) ; - register_ptr_to_python>(); - ExceptionClass( "Cancelled", PyExc_RuntimeError ) .def( init<>() ) ; From 2100fb41c5986a1aa002f7555957e4aa2236945f Mon Sep 17 00:00:00 2001 From: John Haddon Date: Tue, 17 Feb 2026 16:16:04 +0000 Subject: [PATCH 2/3] Canceller : Allow cancellers to be chained This means that one canceller can automatically cancel others. It will be used in Gaffer, where multiple clients (each with their own canceller) may end up working on the same compute by virtue of task collaboration. Although this adds a mutex and additional state, the hot-path that we care about (`check()`), is unchanged. The mutex is only used rarely, when adding/removing children and at the point of cancellation. --- Changes | 4 +- include/IECore/Canceller.h | 72 ++++++++++++++++- src/IECorePython/CancellerBinding.cpp | 57 ++++++++++++-- test/IECore/CancellerTest.py | 106 ++++++++++++++++++++++++++ 4 files changed, 229 insertions(+), 10 deletions(-) diff --git a/Changes b/Changes index 6f5d39cdc0..dee3877b30 100644 --- a/Changes +++ b/Changes @@ -4,7 +4,9 @@ Improvements ------------ -- Canceller : Derived from RefCounted, to allow Cancellers to be shared between multiple clients. +- Canceller : + - Derived from RefCounted, to allow Cancellers to be shared between multiple clients. + - Enabled chaining of Cancellers, via `addChild()` and `removeChild()` methods and a `ScopedChild` utility class. - MessageHandler : Added `msg()` overload that accepts a `fmt::format_string` and arguments. - InternedString : Added specialisation for `fmt::formatter`. diff --git a/include/IECore/Canceller.h b/include/IECore/Canceller.h index 44b9438656..d464250c6c 100644 --- a/include/IECore/Canceller.h +++ b/include/IECore/Canceller.h @@ -39,6 +39,7 @@ #include #include +#include namespace IECore { @@ -93,7 +94,14 @@ class IECORE_API Canceller : public IECore::RefCounted ); // Set cancellation flag _after_ storing time, so that // `elapsedTime()` always sees a valid time. - m_cancelled = true; + if( !m_cancelled.exchange( true ) ) + { + std::lock_guard lock( m_childrenMutex ); + for( const auto &[child, count] : m_children ) + { + child->cancel(); + } + }; } bool cancelled() const @@ -125,13 +133,75 @@ class IECORE_API Canceller : public IECore::RefCounted } } + /// Adds a child canceller that will be cancelled automatically + /// when this is cancelled. If this is already cancels, then the + /// child is cancelled immediately. + void addChild( const Ptr &child ) + { + std::lock_guard lock( m_childrenMutex ); + m_children[child]++; + if( m_cancelled ) + { + child->cancel(); + } + } + + /// Removes a child canceller. Additions are counted, and actual + /// removal only occurs when the number of removals equals the number + /// of additions. + void removeChild( const Ptr &child ) + { + std::lock_guard lock( m_childrenMutex ); + auto it = m_children.find( child ); + if( it != m_children.end() ) + { + if( --it->second == 0 ) + { + m_children.erase( it ); + } + } + } + + /// Convenience class to manage removal of children in an + /// exception-safe way. + class IECORE_API ScopedChild : boost::noncopyable + { + + public : + + /// Adds `child` to `parent`. + ScopedChild( Canceller *parent, const Ptr &child ) + : m_parent( parent ), m_child( child ) + { + m_parent->addChild( m_child ); + } + + /// Removes `child` from `parent`. + ~ScopedChild() + { + m_parent->removeChild( m_child ); + } + + private : + + Canceller *m_parent; + Ptr m_child; + + }; + + private : std::atomic_bool m_cancelled; std::atomic m_cancellationTime; + std::mutex m_childrenMutex; + std::unordered_map m_children; + }; +IE_CORE_DECLAREPTR( Canceller ) + }; // namespace IECore #endif // IECORE_CANCELLER_H diff --git a/src/IECorePython/CancellerBinding.cpp b/src/IECorePython/CancellerBinding.cpp index 5b8bb19a8c..fce926d8cc 100644 --- a/src/IECorePython/CancellerBinding.cpp +++ b/src/IECorePython/CancellerBinding.cpp @@ -42,6 +42,8 @@ #include "IECore/Canceller.h" +#include + using namespace boost::python; using namespace IECore; @@ -53,6 +55,35 @@ double elapsedTimeWrapper( const Canceller &canceller ) return std::chrono::duration( canceller.elapsedTime() ).count(); } +class ScopedChildWrapper : public boost::noncopyable +{ + + public : + + ScopedChildWrapper( Canceller &parent, Canceller &child ) + : m_parent( &parent ), m_child( &child ) + { + } + + void enter() + { + m_scope.emplace( m_parent.get(), m_child ); + } + + bool exit( boost::python::object excType, boost::python::object excValue, boost::python::object excTraceBack ) + { + m_scope.reset(); + return false; // Don't suppress exceptions + } + + private : + + CancellerPtr m_parent; + CancellerPtr m_child; + std::optional m_scope; + +}; + } // namespace namespace IECorePython @@ -61,14 +92,24 @@ namespace IECorePython void bindCanceller() { - RefCountedClass( "Canceller" ) - .def( init<>() ) - .def( "cancel", &Canceller::cancel ) - .def( "cancelled", &Canceller::cancelled ) - .def( "check", &Canceller::check ) - .staticmethod( "check" ) - .def( "elapsedTime", &elapsedTimeWrapper ) - ; + { + scope s = RefCountedClass( "Canceller" ) + .def( init<>() ) + .def( "cancel", &Canceller::cancel ) + .def( "cancelled", &Canceller::cancelled ) + .def( "check", &Canceller::check ) + .staticmethod( "check" ) + .def( "elapsedTime", &elapsedTimeWrapper ) + .def( "addChild", &Canceller::addChild ) + .def( "removeChild", &Canceller::removeChild ) + ; + + class_( "ScopedChild", no_init ) + .def( init() ) + .def( "__enter__", &ScopedChildWrapper::enter, return_self<>() ) + .def( "__exit__", &ScopedChildWrapper::exit ) + ; + } ExceptionClass( "Cancelled", PyExc_RuntimeError ) .def( init<>() ) diff --git a/test/IECore/CancellerTest.py b/test/IECore/CancellerTest.py index 248145e9a7..4659dca14d 100644 --- a/test/IECore/CancellerTest.py +++ b/test/IECore/CancellerTest.py @@ -168,5 +168,111 @@ def testExceptionLifetime( self ) : del exception self.assertIsNone( w() ) + def testAddChild( self ) : + + parent = IECore.Canceller() + child = IECore.Canceller() + + parent.addChild( child ) + self.assertFalse( parent.cancelled() ) + self.assertFalse( child.cancelled() ) + + parent.cancel() + self.assertTrue( parent.cancelled() ) + self.assertTrue( child.cancelled() ) + + def testChildCancelledIfParentAlreadyCancelled( self ) : + + parent = IECore.Canceller() + parent.cancel() + + child = IECore.Canceller() + parent.addChild( child ) + self.assertTrue( child.cancelled() ) + + def testRemoveChild( self ) : + + parent = IECore.Canceller() + child = IECore.Canceller() + + parent.addChild( child ) + parent.removeChild( child ) + + parent.cancel() + self.assertTrue( parent.cancelled() ) + self.assertFalse( child.cancelled() ) + + def testChildCount( self ) : + + parent = IECore.Canceller() + child = IECore.Canceller() + + parent.addChild( child ) + parent.addChild( child ) + parent.removeChild( child ) + + parent.cancel() + self.assertTrue( parent.cancelled() ) + self.assertTrue( child.cancelled() ) + + parent = IECore.Canceller() + child = IECore.Canceller() + + parent.addChild( child ) + parent.addChild( child ) + parent.removeChild( child ) + parent.removeChild( child ) + + parent.cancel() + self.assertTrue( parent.cancelled() ) + self.assertFalse( child.cancelled() ) + + def testChildCycles( self ) : + + canceller1 = IECore.Canceller() + canceller2 = IECore.Canceller() + canceller1.addChild( canceller2 ) + canceller2.addChild( canceller1 ) + + # This should return quickly despite the infinite parent->child->parent + # loop, because it can stop as soon as everything is cancelled. + canceller1.cancel() + self.assertTrue( canceller1.cancelled() ) + self.assertTrue( canceller2.cancelled() ) + + # In this case, the client is responsible for removing reference + # cycles to allow destruction. + canceller1.removeChild( canceller2 ) + self.assertEqual( canceller2.refCount(), 1 ) + self.assertEqual( canceller1.refCount(), 2 ) + + def testScopedChild( self ) : + + parent = IECore.Canceller() + child = IECore.Canceller() + + with IECore.Canceller.ScopedChild( parent, child ) : + parent.cancel() + self.assertTrue( parent.cancelled() ) + self.assertTrue( child.cancelled() ) + + parent = IECore.Canceller() + child = IECore.Canceller() + with IECore.Canceller.ScopedChild( parent, child ) : + pass + parent.cancel() + + self.assertTrue( parent.cancelled() ) + self.assertFalse( child.cancelled() ) + + parent = IECore.Canceller() + child = IECore.Canceller() + with IECore.Canceller.ScopedChild( parent, child ) : + with IECore.Canceller.ScopedChild( parent, child ) : + pass + parent.cancel() + self.assertTrue( parent.cancelled() ) + self.assertTrue( child.cancelled() ) + if __name__ == "__main__": unittest.main() From 519cc901416ab01720656453d2139d68a4ecc2f6 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 18 Feb 2026 11:07:41 +0000 Subject: [PATCH 3/3] Canceller : Move implementation to `.inl` file So that the public API fits easily onto a single screen, without being cluttered with implementation detail. --- include/IECore/Canceller.h | 93 ++++--------------------- include/IECore/Canceller.inl | 130 +++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 80 deletions(-) create mode 100644 include/IECore/Canceller.inl diff --git a/include/IECore/Canceller.h b/include/IECore/Canceller.h index d464250c6c..10863a342d 100644 --- a/include/IECore/Canceller.h +++ b/include/IECore/Canceller.h @@ -76,91 +76,30 @@ class IECORE_API Canceller : public IECore::RefCounted public : - Canceller() - : m_cancelled( false ), m_cancellationTime( 0 ) - { - } + Canceller(); IE_CORE_DECLAREMEMBERPTR( Canceller ) - void cancel() - { - // Store time of first cancellation. We use `compare_exchange_weak()` - // to avoid unwanted updates on subsequent calls. - std::chrono::steady_clock::rep epoch( 0 ); - m_cancellationTime.compare_exchange_weak( - epoch, - std::chrono::steady_clock::now().time_since_epoch().count() - ); - // Set cancellation flag _after_ storing time, so that - // `elapsedTime()` always sees a valid time. - if( !m_cancelled.exchange( true ) ) - { - std::lock_guard lock( m_childrenMutex ); - for( const auto &[child, count] : m_children ) - { - child->cancel(); - } - }; - } - - bool cancelled() const - { - /// \todo Can we reduce overhead by reading - /// with a more relaxed memory ordering here? - return m_cancelled; - } + void cancel(); + bool cancelled() const; - static void check( const Canceller *canceller ) - { - if( canceller && canceller->cancelled() ) - { - throw Cancelled(); - } - } + /// Throws `IECore::Cancelled` if `canceller` is non-null and has + /// been cancelled, otherwise does nothing. + static void check( const Canceller *canceller ); /// Returns the time passed since `cancel()` was first called, or `0` if /// it has not been called yet. - std::chrono::steady_clock::duration elapsedTime() const - { - if( m_cancelled ) - { - return std::chrono::steady_clock::now() - std::chrono::steady_clock::time_point( std::chrono::steady_clock::duration( m_cancellationTime ) ); - } - else - { - return std::chrono::steady_clock::duration( 0 ); - } - } + std::chrono::steady_clock::duration elapsedTime() const; /// Adds a child canceller that will be cancelled automatically /// when this is cancelled. If this is already cancels, then the /// child is cancelled immediately. - void addChild( const Ptr &child ) - { - std::lock_guard lock( m_childrenMutex ); - m_children[child]++; - if( m_cancelled ) - { - child->cancel(); - } - } + void addChild( const Ptr &child ); /// Removes a child canceller. Additions are counted, and actual /// removal only occurs when the number of removals equals the number /// of additions. - void removeChild( const Ptr &child ) - { - std::lock_guard lock( m_childrenMutex ); - auto it = m_children.find( child ); - if( it != m_children.end() ) - { - if( --it->second == 0 ) - { - m_children.erase( it ); - } - } - } + void removeChild( const Ptr &child ); /// Convenience class to manage removal of children in an /// exception-safe way. @@ -170,17 +109,9 @@ class IECORE_API Canceller : public IECore::RefCounted public : /// Adds `child` to `parent`. - ScopedChild( Canceller *parent, const Ptr &child ) - : m_parent( parent ), m_child( child ) - { - m_parent->addChild( m_child ); - } - + ScopedChild( Canceller *parent, const Ptr &child ); /// Removes `child` from `parent`. - ~ScopedChild() - { - m_parent->removeChild( m_child ); - } + ~ScopedChild(); private : @@ -204,4 +135,6 @@ IE_CORE_DECLAREPTR( Canceller ) }; // namespace IECore +#include "IECore/Canceller.inl" + #endif // IECORE_CANCELLER_H diff --git a/include/IECore/Canceller.inl b/include/IECore/Canceller.inl new file mode 100644 index 0000000000..ddde11169e --- /dev/null +++ b/include/IECore/Canceller.inl @@ -0,0 +1,130 @@ +////////////////////////////////////////////////////////////////////////// +// +// Copyright (c) 2018, Image Engine Design Inc. All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// +// * Redistributions in binary form must reproduce the above copyright +// notice, this list of conditions and the following disclaimer in the +// documentation and/or other materials provided with the distribution. +// +// * Neither the name of Image Engine Design nor the names of any +// other contributors to this software may be used to endorse or +// promote products derived from this software without specific prior +// written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS +// IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +// THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR +// PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR +// CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, +// EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, +// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR +// PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF +// LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING +// NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +// SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +// +////////////////////////////////////////////////////////////////////////// + +#ifndef IECORE_CANCELLER_INL +#define IECORE_CANCELLER_INL + +namespace IECore +{ + +inline Canceller::Canceller() + : m_cancelled( false ), m_cancellationTime( 0 ) +{ +} + +inline void Canceller::cancel() +{ + // Store time of first cancellation. We use `compare_exchange_weak()` + // to avoid unwanted updates on subsequent calls. + std::chrono::steady_clock::rep epoch( 0 ); + m_cancellationTime.compare_exchange_weak( + epoch, + std::chrono::steady_clock::now().time_since_epoch().count() + ); + // Set cancellation flag _after_ storing time, so that + // `elapsedTime()` always sees a valid time. + if( !m_cancelled.exchange( true ) ) + { + std::lock_guard lock( m_childrenMutex ); + for( const auto &[child, count] : m_children ) + { + child->cancel(); + } + }; +} + +inline bool Canceller::cancelled() const +{ + /// \todo Can we reduce overhead by reading + /// with a more relaxed memory ordering here? + return m_cancelled; +} + +inline void Canceller::check( const Canceller *canceller ) +{ + if( canceller && canceller->cancelled() ) + { + throw Cancelled(); + } +} + +inline std::chrono::steady_clock::duration Canceller::elapsedTime() const +{ + if( m_cancelled ) + { + return std::chrono::steady_clock::now() - std::chrono::steady_clock::time_point( std::chrono::steady_clock::duration( m_cancellationTime ) ); + } + else + { + return std::chrono::steady_clock::duration( 0 ); + } +} + +inline void Canceller::addChild( const Ptr &child ) +{ + std::lock_guard lock( m_childrenMutex ); + m_children[child]++; + if( m_cancelled ) + { + child->cancel(); + } +} + +inline void Canceller::removeChild( const Ptr &child ) +{ + std::lock_guard lock( m_childrenMutex ); + auto it = m_children.find( child ); + if( it != m_children.end() ) + { + if( --it->second == 0 ) + { + m_children.erase( it ); + } + } +} + +inline Canceller::ScopedChild::ScopedChild( Canceller *parent, const Ptr &child ) + : m_parent( parent ), m_child( child ) +{ + m_parent->addChild( m_child ); +} + +inline Canceller::ScopedChild::~ScopedChild() +{ + m_parent->removeChild( m_child ); +} + +}; // namespace IECore + +#endif // IECORE_CANCELLER_INL