diff --git a/ApplicationLibCode/Application/Tools/Telemetry/RiaOpenTelemetryManager.cpp b/ApplicationLibCode/Application/Tools/Telemetry/RiaOpenTelemetryManager.cpp index 967c906b94..73c78ba867 100644 --- a/ApplicationLibCode/Application/Tools/Telemetry/RiaOpenTelemetryManager.cpp +++ b/ApplicationLibCode/Application/Tools/Telemetry/RiaOpenTelemetryManager.cpp @@ -41,6 +41,7 @@ #include #include #include +#include //-------------------------------------------------------------------------------------------------- /// diff --git a/ApplicationLibCode/Commands/RicCloseSummaryCaseFeature.cpp b/ApplicationLibCode/Commands/RicCloseSummaryCaseFeature.cpp index 8a67ddb94a..10d3e65933 100644 --- a/ApplicationLibCode/Commands/RicCloseSummaryCaseFeature.cpp +++ b/ApplicationLibCode/Commands/RicCloseSummaryCaseFeature.cpp @@ -37,12 +37,9 @@ #include "RiuPlotMainWindow.h" -#include "cafAsyncObjectDeleter.h" #include "cafPdmObjectHandleTools.h" #include "cafSelectionManager.h" -#include "cafAssert.h" - #include CAF_CMD_SOURCE_INIT( RicCloseSummaryCaseFeature, "RicCloseSummaryCaseFeature" ); @@ -96,8 +93,7 @@ void RicCloseSummaryCaseFeature::deleteSummaryCases( std::vectormainPlotWindow(); mainPlotWindow->updateMultiPlotToolBar(); - caf::AsyncPdmObjectVectorDeleter summaryCaseDeleter( cases ); - CAF_ASSERT( cases.empty() ); // vector should be empty immediately. + caf::PdmObjectHandleTools::deleteObjects( cases ); } //-------------------------------------------------------------------------------------------------- diff --git a/ApplicationLibCode/ProjectDataModel/Summary/Ensemble/RimSummaryFileSetEnsemble.cpp b/ApplicationLibCode/ProjectDataModel/Summary/Ensemble/RimSummaryFileSetEnsemble.cpp index 8df10757b5..f18e288c5f 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/Ensemble/RimSummaryFileSetEnsemble.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/Ensemble/RimSummaryFileSetEnsemble.cpp @@ -152,14 +152,7 @@ void RimSummaryFileSetEnsemble::onFileSetNameChanged( const caf::SignalEmitter* //-------------------------------------------------------------------------------------------------- void RimSummaryFileSetEnsemble::createSummaryCasesFromEnsembleFileSet( bool notifyChange ) { - // Delete old cases synchronously to avoid a race between the std::thread spawned by - // deleteChildrenAsync() and the OpenMP parallel loop in loadFileSummaryCaseData(). - // #pragma omp critical does not protect against std::thread races. - // clearWithoutDelete() disconnects observer signals first (preserves the fix for #12262). - auto casesToDelete = m_cases.childrenByType(); - m_cases.clearWithoutDelete(); - for ( auto* c : casesToDelete ) - delete c; + m_cases.deleteChildren(); if ( m_ensembleFileSet() ) { diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp index e3a0f64bcb..c6c99f1e4b 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp @@ -143,8 +143,8 @@ RimSummaryCaseMainCollection::RimSummaryCaseMainCollection() //-------------------------------------------------------------------------------------------------- RimSummaryCaseMainCollection::~RimSummaryCaseMainCollection() { - m_cases.deleteChildrenAsync(); - m_ensembles.deleteChildrenAsync(); + m_cases.deleteChildren(); + m_ensembles.deleteChildren(); } //-------------------------------------------------------------------------------------------------- diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.cpp index e6f95c3364..633d6ff53a 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.cpp @@ -116,7 +116,8 @@ RimSummaryEnsemble::RimSummaryEnsemble() //-------------------------------------------------------------------------------------------------- RimSummaryEnsemble::~RimSummaryEnsemble() { - m_cases.deleteChildrenAsync(); + m_cases.deleteChildren(); + updateReferringCurveSets(); } @@ -204,13 +205,7 @@ RimSummaryCase* RimSummaryEnsemble::firstSummaryCase() const //-------------------------------------------------------------------------------------------------- void RimSummaryEnsemble::replaceCases( const std::vector& summaryCases, bool notifyChange ) { - // Delete synchronously to prevent a race between the std::thread spawned by - // deleteChildrenAsync() and any active OpenMP parallel loop in loadFileSummaryCaseData(). - // clearWithoutDelete() disconnects observer signals first (preserves the fix for #12262). - auto casesToDelete = m_cases.childrenByType(); - m_cases.clearWithoutDelete(); - for ( auto* c : casesToDelete ) - delete c; + m_cases.deleteChildren(); if ( summaryCases.empty() ) return; diff --git a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/CMakeLists.txt b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/CMakeLists.txt index 2d4de6e8ad..0999f4f589 100644 --- a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/CMakeLists.txt +++ b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/CMakeLists.txt @@ -56,8 +56,6 @@ set(PROJECT_FILES cafTristate.h cafFilePath.cpp cafFilePath.h - cafAsyncObjectDeleter.h - cafAsyncObjectDeleter.inl cafAsyncWorkerManager.h cafAsyncWorkerManager.cpp cafPdmObjectHandleTools.h diff --git a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAsyncObjectDeleter.h b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAsyncObjectDeleter.h deleted file mode 100644 index 705e081e56..0000000000 --- a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAsyncObjectDeleter.h +++ /dev/null @@ -1,55 +0,0 @@ -//################################################################################################## -// -// Custom Visualization Core library -// Copyright (C) 2011-2013 Ceetron AS -// -// This library may be used under the terms of either the GNU General Public License or -// the GNU Lesser General Public License as follows: -// -// GNU General Public License Usage -// This library is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This library is distributed in the hope that it will be useful, but WITHOUT ANY -// WARRANTY; without even the implied warranty of MERCHANTABILITY or -// FITNESS FOR A PARTICULAR PURPOSE. -// -// See the GNU General Public License at <> -// for more details. -// -// GNU Lesser General Public License Usage -// This library is free software; you can redistribute it and/or modify -// it under the terms of the GNU Lesser General Public License as published by -// the Free Software Foundation; either version 2.1 of the License, or -// (at your option) any later version. -// -// This library is distributed in the hope that it will be useful, but WITHOUT ANY -// WARRANTY; without even the implied warranty of MERCHANTABILITY or -// FITNESS FOR A PARTICULAR PURPOSE. -// -// See the GNU Lesser General Public License at <> -// for more details. -// -//################################################################################################## - -#pragma once - -namespace caf -{ -template -class AsyncPdmObjectVectorDeleter -{ -public: - AsyncPdmObjectVectorDeleter( std::vector& pointerVector ); - AsyncPdmObjectVectorDeleter( std::vector>& pdmPointerVector ); - ~AsyncPdmObjectVectorDeleter(); - void start(); - -private: - std::vector m_pointersToDelete; -}; -} // namespace caf - -#include "cafAsyncObjectDeleter.inl" diff --git a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAsyncObjectDeleter.inl b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAsyncObjectDeleter.inl deleted file mode 100644 index 6d64656b61..0000000000 --- a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAsyncObjectDeleter.inl +++ /dev/null @@ -1,76 +0,0 @@ -#include - -#include "cafAsyncWorkerManager.h" -#include "cafPdmObjectHandle.h" - -namespace caf -{ -//-------------------------------------------------------------------------------------------------- -/// Constructor that takes ownership of the data in the provided vector -//-------------------------------------------------------------------------------------------------- -template -AsyncPdmObjectVectorDeleter::AsyncPdmObjectVectorDeleter( std::vector& pointerVector ) -{ - m_pointersToDelete.reserve( pointerVector.size() ); - for ( PdmObjectType* rawPointer : pointerVector ) - { - if ( rawPointer ) - { - PdmObjectHandle* objectHandle = static_cast( rawPointer ); - objectHandle->prepareForDelete(); - m_pointersToDelete.push_back( objectHandle ); - } - } - pointerVector.clear(); -} - -//-------------------------------------------------------------------------------------------------- -/// Constructor that takes ownership of the data in the provided vector -//-------------------------------------------------------------------------------------------------- -template -AsyncPdmObjectVectorDeleter::AsyncPdmObjectVectorDeleter( std::vector>& pdmPointerVector ) -{ - m_pointersToDelete.reserve( pdmPointerVector.size() ); - for ( PdmPointer& pdmPointer : pdmPointerVector ) - { - if ( pdmPointer.notNull() ) - { - PdmObjectHandle* objectHandle = pdmPointer.rawPtr(); - objectHandle->prepareForDelete(); - m_pointersToDelete.push_back( objectHandle ); - } - } - pdmPointerVector.clear(); -} - -//-------------------------------------------------------------------------------------------------- -/// Destructor will launch the asynchronous deletion if start() hasn't already been run. -//-------------------------------------------------------------------------------------------------- -template -AsyncPdmObjectVectorDeleter::~AsyncPdmObjectVectorDeleter() -{ - if ( !m_pointersToDelete.empty() ) - { - start(); - } -} - -//-------------------------------------------------------------------------------------------------- -/// Perform deletion of the pointers in a separate thread. -//-------------------------------------------------------------------------------------------------- -template -void AsyncPdmObjectVectorDeleter::start() -{ - std::thread thread( - []( std::vector&& pointerVector ) - { - for ( PdmObjectHandle* pointerToDelete : pointerVector ) - { - delete pointerToDelete; - } - }, - std::move( m_pointersToDelete ) ); - AsyncWorkerManager::instance().takeThreadOwnership( thread, false ); -} - -} // namespace caf \ No newline at end of file diff --git a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmChildArrayField.h b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmChildArrayField.h index dc29e0b02b..8a6213fe71 100644 --- a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmChildArrayField.h +++ b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmChildArrayField.h @@ -71,8 +71,6 @@ public: void setValue( const std::vector& objects ); size_t indexOf( const PdmObjectHandle* obj ) const override; - virtual void deleteChildrenAsync(); - // std::vector-like access DataType* operator[]( size_t index ) const; diff --git a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmChildArrayField.inl b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmChildArrayField.inl index d05c0bc20d..b2391a16a5 100644 --- a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmChildArrayField.inl +++ b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmChildArrayField.inl @@ -1,4 +1,3 @@ -#include "cafAsyncObjectDeleter.h" #include "cafClassTypeName.h" #include "cafPdmObjectHandle.h" @@ -184,26 +183,6 @@ void PdmChildArrayField::deleteChildren() m_pointers.clear(); } -//-------------------------------------------------------------------------------------------------- -/// Transfers ownership of the objects pointed to a separate thread. -/// Then clears the container and lets the thread delete the objects. -//-------------------------------------------------------------------------------------------------- -template -void PdmChildArrayField::deleteChildrenAsync() -{ - CAF_ASSERT( isInitializedByInitFieldMacro() ); - - auto objectsToDelete = m_pointers; - - // Disconnect the connection to observers before deleting the objects - // https://github.com/OPM/ResInsight/issues/12262 - // - // See test in \Fwk\AppFwk\cafProjectDataModel\cafPdmCore\cafPdmCore_UnitTests\cafPdmChildArrayFieldHandleTest.cpp - clearWithoutDelete(); - - AsyncPdmObjectVectorDeleter pointerDeleter( objectsToDelete ); -} - //-------------------------------------------------------------------------------------------------- /// Removes the pointer at index from the container. Does not delete the object pointed to. //-------------------------------------------------------------------------------------------------- diff --git a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmCore_UnitTests/cafPdmChildArrayFieldHandleTest.cpp b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmCore_UnitTests/cafPdmChildArrayFieldHandleTest.cpp index ea68623988..695ce16ae1 100644 --- a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmCore_UnitTests/cafPdmChildArrayFieldHandleTest.cpp +++ b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmCore_UnitTests/cafPdmChildArrayFieldHandleTest.cpp @@ -160,15 +160,14 @@ TEST( ChildArrayFieldHandle, DerivedOtherObjects ) delete containerObj; } -TEST( ChildArrayFieldHandle, AsyncDeleteOfMultipleChildren ) +TEST( ChildArrayFieldHandle, DeletedChildrenDisconnectFromObserver ) { - // https://github.com/OPM/ResInsight/issues/12262 + // A deleted child unregisters itself from its observers. ~Signal() calls removeObservedSignal() + // on every observer, so the container is not left holding signals owned by destroyed objects. // - // To trigger crash, remove the line - // clearWithoutDelete(); - // from PdmChildArrayField::deleteChildrenAsync() - // - // Crash was reproduced in Debug build in VS2022 17.13.2 + // https://github.com/OPM/ResInsight/issues/12262 was a race in the async delete path, which no + // longer exists. ~Signal() ran on a worker thread and mutated the observer list concurrently + // with the main thread. Deletion is synchronous now, so this ordering is deterministic. ContainerObj containerObj; const int numObjects = 1000; @@ -177,8 +176,9 @@ TEST( ChildArrayFieldHandle, AsyncDeleteOfMultipleChildren ) containerObj.createAndAppendObject(); } - containerObj.derivedObjs.deleteChildrenAsync(); + EXPECT_EQ( size_t( numObjects ), containerObj.observedSignals().size() ); - // Wait for async delete to complete - std::this_thread::sleep_for( std::chrono::milliseconds( 100 ) ); + containerObj.derivedObjs.deleteChildren(); + + EXPECT_EQ( size_t( 0 ), containerObj.observedSignals().size() ); } diff --git a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmObjectHandleTools.h b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmObjectHandleTools.h index f7182eeef7..7eda7049eb 100644 --- a/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmObjectHandleTools.h +++ b/Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmObjectHandleTools.h @@ -33,5 +33,22 @@ namespace PdmObjectHandleTools return destinationObjects; } + + // Delete all objects in the vector, then clear it. + // + // Use this instead of a hand written delete loop. Objects that are still owned by a + // PdmChildArrayField must not be deleted this way, call deleteChildren() on the field instead so + // the observer signals are disconnected first. + // + template + void deleteObjects( std::vector& objects ) + { + for ( auto* object : objects ) + { + delete object; + } + + objects.clear(); + } } //namespace PdmObjectHandleTools } //namespace caf diff --git a/GrpcInterface/RiaGrpcServer.cpp b/GrpcInterface/RiaGrpcServer.cpp index d8af55124d..9b822f7962 100644 --- a/GrpcInterface/RiaGrpcServer.cpp +++ b/GrpcInterface/RiaGrpcServer.cpp @@ -38,6 +38,8 @@ #include +#include + using grpc::CompletionQueue; using grpc::Server; using grpc::ServerAsyncResponseWriter;