From a1f2446df019221fe877c63d20baf7d5345253bd Mon Sep 17 00:00:00 2001 From: Marcin Nowak <Marcin.Nowak@cern.ch> Date: Thu, 9 Jul 2020 19:27:35 +0200 Subject: [PATCH] Add locking to the source AuxContainer when making a copy of it ATEAM-624 and ATEAM-626 show job failures that we suspect were caused by modifying AuxSelection at the same time other thread tried to make a copy of the same AuxContainer. --- Event/xAOD/xAODCore/Root/AuxContainerBase.cxx | 17 ++++++++++------- Event/xAOD/xAODCore/Root/AuxInfoBase.cxx | 17 +++++++++++------ .../xAOD/xAODCore/Root/ShallowAuxContainer.cxx | 16 ++++++++++------ 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/Event/xAOD/xAODCore/Root/AuxContainerBase.cxx b/Event/xAOD/xAODCore/Root/AuxContainerBase.cxx index 361aca988a3..c1da5654a1b 100644 --- a/Event/xAOD/xAODCore/Root/AuxContainerBase.cxx +++ b/Event/xAOD/xAODCore/Root/AuxContainerBase.cxx @@ -48,21 +48,22 @@ namespace xAOD { /// AuxContainerBase::AuxContainerBase( const AuxContainerBase& parent ) : SG::IAuxStore(), - m_selection( parent.m_selection ), - m_compression( parent.m_compression ), m_auxids(), m_vecs(), m_store( 0 ), m_storeIO( 0 ), m_ownsStore( true ), - m_locked( false ), - m_name( parent.m_name ) { + m_locked( false ) + { + // Keep the source unmutable during copy + guard_t guard( parent.m_mutex ); + m_selection = parent.m_selection; + m_compression = parent.m_compression; + m_name = parent.m_name; // Unfortunately the dynamic variables can not be copied this easily... if( parent.m_store ) { // cppcheck-suppress copyCtorPointerCopying - m_store = parent.m_store; m_ownsStore = false; + m_store = parent.m_store; m_storeIO = dynamic_cast< SG::IAuxStoreIO* >( m_store ); - m_selection = parent.m_selection; - m_compression = parent.m_compression; m_auxids = m_store->getAuxIDs(); } } @@ -111,6 +112,8 @@ namespace xAOD { // Protect against self-assignment: if( this == &rhs ) return *this; + // Keep the source unmutable during copy + guard_t guard( rhs.m_mutex ); m_selection = rhs.m_selection; m_compression = rhs.m_compression; diff --git a/Event/xAOD/xAODCore/Root/AuxInfoBase.cxx b/Event/xAOD/xAODCore/Root/AuxInfoBase.cxx index 7b436ed8022..65a86d6a007 100644 --- a/Event/xAOD/xAODCore/Root/AuxInfoBase.cxx +++ b/Event/xAOD/xAODCore/Root/AuxInfoBase.cxx @@ -47,19 +47,21 @@ namespace xAOD { /// AuxInfoBase::AuxInfoBase( const AuxInfoBase& parent ) : SG::IAuxStore(), - m_selection( parent.m_selection ), m_auxids(), m_vecs(), m_store( 0 ), m_storeIO( 0 ), m_ownsStore( true ), - m_locked( false ), - m_name( parent.m_name ) { + m_locked( false ) + { + // Keep the source unmutable during copy + guard_t guard( parent.m_mutex ); + m_selection = parent.m_selection; + m_name = parent.m_name; // Unfortunately the dynamic variables can not be copied this easily... if( parent.m_store ) { // cppcheck-suppress copyCtorPointerCopying - m_store = parent.m_store; m_ownsStore = false; + m_store = parent.m_store; m_storeIO = dynamic_cast< SG::IAuxStoreIO* >( m_store ); - m_selection = parent.m_selection; m_auxids.insert( m_store->getAuxIDs().begin(), m_store->getAuxIDs().end() ); } @@ -109,6 +111,9 @@ namespace xAOD { // Protect against self-assignment: if( this == &rhs ) return *this; + // Keep the source unmutable during copy + std::scoped_lock lck{m_mutex, rhs.m_mutex}; + m_selection = rhs.m_selection; // Clean up after the old dynamic store: @@ -122,8 +127,8 @@ namespace xAOD { // Take posession of the new dynamic store: if( rhs.m_store ) { - m_store = rhs.m_store; m_ownsStore = false; + m_store = rhs.m_store; m_storeIO = dynamic_cast< SG::IAuxStoreIO* >( m_store ); m_auxids.insert (m_store->getAuxIDs()); } diff --git a/Event/xAOD/xAODCore/Root/ShallowAuxContainer.cxx b/Event/xAOD/xAODCore/Root/ShallowAuxContainer.cxx index 40102f76d8f..5db106afb16 100644 --- a/Event/xAOD/xAODCore/Root/ShallowAuxContainer.cxx +++ b/Event/xAOD/xAODCore/Root/ShallowAuxContainer.cxx @@ -35,15 +35,17 @@ namespace xAOD { ShallowAuxContainer::ShallowAuxContainer( const ShallowAuxContainer& parent ) : SG::IAuxStore(), SG::IAuxStoreIO(), SG::IAuxStoreHolder(), - m_selection( parent.m_selection ), m_store( parent.m_store ), m_storeIO( parent.m_storeIO ), m_ownsStore( false ), m_locked( parent.m_locked ), m_parentLink( parent.m_parentLink ), m_parentIO( parent.m_parentIO ), m_shallowIO( parent.m_shallowIO ), m_auxids (), - m_auxidsValid (false), - m_name( parent.m_name ) { - + m_auxidsValid (false) + { + // Keep the source unmutable during copy + guard_t guard( parent.m_mutex ); + m_selection = parent.m_selection; + m_name = parent.m_name; } /// @param parent The parent object to make a shallow copy of @@ -75,12 +77,14 @@ namespace xAOD { ShallowAuxContainer& ShallowAuxContainer::operator= ( const ShallowAuxContainer& rhs ) { - guard_t guard (m_mutex); // Check if anything needs to be done: if( this == &rhs ) { return *this; } + // Keep the source unmutable during copy + std::scoped_lock lck{m_mutex, rhs.m_mutex}; + // Clean up if necessary: if( m_ownsStore && m_store ) { delete m_store; @@ -88,9 +92,9 @@ namespace xAOD { } m_selection = rhs.m_selection; + m_ownsStore = false; m_store = rhs.m_store; m_storeIO = rhs.m_storeIO; - m_ownsStore = false; m_locked = rhs.m_locked; m_parentLink = rhs.m_parentLink; m_parentIO = rhs.m_parentIO; -- GitLab