Remove AsyncPdmObjectVectorDeleter and delete PDM objects synchronously

Deleting PDM objects on a worker thread is not safe. PdmObjectHandle::prepareForDelete()
mutates state owned by other objects, it nulls the guarded pointers held by other objects
and clears m_pointersReferencingMe, an unsynchronised std::set that every PdmPointer
construction and destruction touches. Destroying an object off the main thread therefore
races with the main thread on the shared object graph. See issue 14491.

Profiling a summary ensemble teardown shows the mechanism does not pay for itself. Releasing
397 Drogon realizations takes 0.24 s sequentially and 0.24 s in parallel, and for a heavy
case the parallel release is slower than the sequential one, because free() is serialised
inside the allocator. The PDM bookkeeping itself is 0.4 to 2.8 percent of the teardown.

Remove the class and deleteChildrenAsync(), and delete synchronously instead. The call sites
that used clearWithoutDelete() and a manual delete loop to work around the race can now call
deleteChildren() directly. Add caf::PdmObjectHandleTools::deleteObjects() for the case where
the objects are no longer owned by a child array field.

The observer disconnection from issue 12262 does not depend on clearWithoutDelete(). ~Signal()
unregisters itself from every observer, so a deleted child detaches itself. That fix addressed
the async race, where ~Signal() mutated the observer list from a worker thread. The unit test
is updated to assert the observed signal count directly instead of relying on a crash.

ApplicationLibCode/Application/Tools/Telemetry/RiaOpenTelemetryManager.cpp

@@ -41,6 +41,7 @@
 #include <algorithm>
 #include <random>
 #include <sstream>
+#include <thread>
 
 //--------------------------------------------------------------------------------------------------
 ///

ApplicationLibCode/Commands/RicCloseSummaryCaseFeature.cpp

@@ -37,12 +37,9 @@
 
 #include "RiuPlotMainWindow.h"
 
-#include "cafAsyncObjectDeleter.h"
 #include "cafPdmObjectHandleTools.h"
 #include "cafSelectionManager.h"
 
-#include "cafAssert.h"
-
 #include <QAction>
 
 CAF_CMD_SOURCE_INIT( RicCloseSummaryCaseFeature, "RicCloseSummaryCaseFeature" );
@@ -96,8 +93,7 @@ void RicCloseSummaryCaseFeature::deleteSummaryCases( std::vector<RimSummaryCase*
     RiuPlotMainWindow* mainPlotWindow = RiaGuiApplication::instance()->mainPlotWindow();
     mainPlotWindow->updateMultiPlotToolBar();
 
-    caf::AsyncPdmObjectVectorDeleter<RimSummaryCase> summaryCaseDeleter( cases );
-    CAF_ASSERT( cases.empty() ); // vector should be empty immediately.
+    caf::PdmObjectHandleTools::deleteObjects( cases );
 }
 
 //--------------------------------------------------------------------------------------------------

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() )
     {

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();
 }
 
 //--------------------------------------------------------------------------------------------------

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<RimSummaryCase*>& 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;
 

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

Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAsyncObjectDeleter.h

@@ -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 <<http://www.gnu.org/licenses/gpl.html>>
-//   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 <<http://www.gnu.org/licenses/lgpl-2.1.html>>
-//   for more details.
-//
-//##################################################################################################
-
-#pragma once
-
-namespace caf
-{
-template <typename PdmObjectType>
-class AsyncPdmObjectVectorDeleter
-{
-public:
-    AsyncPdmObjectVectorDeleter( std::vector<PdmObjectType*>& pointerVector );
-    AsyncPdmObjectVectorDeleter( std::vector<PdmPointer<PdmObjectType>>& pdmPointerVector );
-    ~AsyncPdmObjectVectorDeleter();
-    void start();
-
-private:
-    std::vector<PdmObjectHandle*> m_pointersToDelete;
-};
-} // namespace caf
-
-#include "cafAsyncObjectDeleter.inl"

Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafAsyncObjectDeleter.inl

@@ -1,76 +0,0 @@
-#include <thread>
-
-#include "cafAsyncWorkerManager.h"
-#include "cafPdmObjectHandle.h"
-
-namespace caf
-{
-//--------------------------------------------------------------------------------------------------
-/// Constructor that takes ownership of the data in the provided vector
-//--------------------------------------------------------------------------------------------------
-template <typename PdmObjectType>
-AsyncPdmObjectVectorDeleter<PdmObjectType>::AsyncPdmObjectVectorDeleter( std::vector<PdmObjectType*>& pointerVector )
-{
-    m_pointersToDelete.reserve( pointerVector.size() );
-    for ( PdmObjectType* rawPointer : pointerVector )
-    {
-        if ( rawPointer )
-        {
-            PdmObjectHandle* objectHandle = static_cast<PdmObjectHandle*>( rawPointer );
-            objectHandle->prepareForDelete();
-            m_pointersToDelete.push_back( objectHandle );
-        }
-    }
-    pointerVector.clear();
-}
-
-//--------------------------------------------------------------------------------------------------
-/// Constructor that takes ownership of the data in the provided vector
-//--------------------------------------------------------------------------------------------------
-template <typename PdmObjectType>
-AsyncPdmObjectVectorDeleter<PdmObjectType>::AsyncPdmObjectVectorDeleter( std::vector<PdmPointer<PdmObjectType>>& pdmPointerVector )
-{
-    m_pointersToDelete.reserve( pdmPointerVector.size() );
-    for ( PdmPointer<PdmObjectType>& 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 <typename PdmObjectType>
-AsyncPdmObjectVectorDeleter<PdmObjectType>::~AsyncPdmObjectVectorDeleter()
-{
-    if ( !m_pointersToDelete.empty() )
-    {
-        start();
-    }
-}
-
-//--------------------------------------------------------------------------------------------------
-/// Perform deletion of the pointers in a separate thread.
-//--------------------------------------------------------------------------------------------------
-template <typename PdmObjectType>
-void AsyncPdmObjectVectorDeleter<PdmObjectType>::start()
-{
-    std::thread thread(
-        []( std::vector<PdmObjectHandle*>&& pointerVector )
-        {
-            for ( PdmObjectHandle* pointerToDelete : pointerVector )
-            {
-                delete pointerToDelete;
-            }
-        },
-        std::move( m_pointersToDelete ) );
-    AsyncWorkerManager::instance().takeThreadOwnership( thread, false );
-}
-
-} // namespace caf

Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmChildArrayField.h

@@ -71,8 +71,6 @@ public:
     void             setValue( const std::vector<DataType*>& objects );
     size_t           indexOf( const PdmObjectHandle* obj ) const override;
 
-    virtual void deleteChildrenAsync();
-
     // std::vector-like access
 
     DataType* operator[]( size_t index ) const;

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<DataType*>::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 <typename DataType>
-void PdmChildArrayField<DataType*>::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<DataType> pointerDeleter( objectsToDelete );
-}
-
 //--------------------------------------------------------------------------------------------------
 /// Removes the pointer at index from the container. Does not delete the object pointed to.
 //--------------------------------------------------------------------------------------------------

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<DataType*>::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() );
 }

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 <typename ObjectType>
+    void deleteObjects( std::vector<ObjectType*>& objects )
+    {
+        for ( auto* object : objects )
+        {
+            delete object;
+        }
+
+        objects.clear();
+    }
 } //namespace PdmObjectHandleTools
 } //namespace caf

GrpcInterface/RiaGrpcServer.cpp

@@ -38,6 +38,8 @@
 
 #include <QTcpServer>
 
+#include <thread>
+
 using grpc::CompletionQueue;
 using grpc::Server;
 using grpc::ServerAsyncResponseWriter;