From c2448b17963acd5fa553f71e859e5d1cebf9c1ce Mon Sep 17 00:00:00 2001 From: Magne Sjaastad Date: Fri, 31 Jul 2026 09:43:33 +0200 Subject: [PATCH 1/6] #14423 Fix crash when closing summary cases referred by a delta ensemble Removing a source case makes a delta ensemble recreate and delete its derived cases. Those cases are part of the list being removed, leaving dangling pointers that crash in PdmObjectHandle::prepareForDelete(). Use guarded pointers and return only the surviving cases. --- .../Summary/RimSummaryCaseMainCollection.cpp | 16 +++- .../Summary/RimSummaryCaseMainCollection.h | 1 + ApplicationLibCode/UnitTests/CMakeLists.txt | 1 + .../RimSummaryCaseMainCollection-Test.cpp | 88 +++++++++++++++++++ 4 files changed, 104 insertions(+), 2 deletions(-) create mode 100644 ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp index c6c99f1e4b8..dc80e2e5fe8 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp @@ -56,6 +56,7 @@ #include "cafCmdFeatureMenuBuilder.h" #include "cafPdmFieldReorderCapability.h" +#include "cafPdmPointer.h" #include "cafProgressInfo.h" #include @@ -228,9 +229,14 @@ void RimSummaryCaseMainCollection::removeCase( RimSummaryCase* summaryCase, bool //-------------------------------------------------------------------------------------------------- void RimSummaryCaseMainCollection::removeCases( std::vector& cases ) { - for ( auto sumCase : cases ) + // Removing one case can delete other cases in the list. A delta ensemble recreates its derived cases when a source + // case is removed, and deletes the derived cases no longer in use. Use guarded pointers to avoid touching deleted + // cases, and return only the cases that are still alive. + std::vector> guardedCases( cases.begin(), cases.end() ); + + for ( const auto& sumCase : guardedCases ) { - removeCase( sumCase, false ); + if ( sumCase.notNull() ) removeCase( sumCase, false ); } for ( RimSummaryEnsemble* ensemble : m_ensembles ) @@ -238,6 +244,12 @@ void RimSummaryCaseMainCollection::removeCases( std::vector& ca ensemble->updateReferringCurveSetsZoomAll(); } + cases.clear(); + for ( const auto& sumCase : guardedCases ) + { + if ( sumCase.notNull() ) cases.push_back( sumCase ); + } + dataSourceHasChanged.send(); } diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.h b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.h index a6e44e950ad..f5d480def0b 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.h +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.h @@ -60,6 +60,7 @@ class RimSummaryCaseMainCollection : public caf::PdmObject void addCases( const std::vector cases ); void addCase( RimSummaryCase* summaryCase ); void removeCase( RimSummaryCase* summaryCase, bool notifyChange = true ); + // Cases deleted as a side effect of the removal are erased from the input vector void removeCases( std::vector& cases ); void moveCase( RimSummaryCase* summaryCase, int destinationIndex ); diff --git a/ApplicationLibCode/UnitTests/CMakeLists.txt b/ApplicationLibCode/UnitTests/CMakeLists.txt index 78d52c60046..8ac5af0cf61 100644 --- a/ApplicationLibCode/UnitTests/CMakeLists.txt +++ b/ApplicationLibCode/UnitTests/CMakeLists.txt @@ -85,6 +85,7 @@ set(SOURCE_UNITTEST_FILES ${CMAKE_CURRENT_LIST_DIR}/RimWellRftPlot-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RimDataFilterCollection-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RimSummaryCaseCollection-Test.cpp + ${CMAKE_CURRENT_LIST_DIR}/RimSummaryCaseMainCollection-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RifActiveCellsReader-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RifCsvDataTableFormatter-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RiaSummaryAddressAnalyzer-Test.cpp diff --git a/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp b/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp new file mode 100644 index 00000000000..d24e8598003 --- /dev/null +++ b/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp @@ -0,0 +1,88 @@ +#include "gtest/gtest.h" + +#include "Summary/RiaSummaryTools.h" + +#include "RigCaseRealizationParameters.h" + +#include "RimDeltaSummaryEnsemble.h" +#include "RimMockSummaryCase.h" +#include "RimSummaryCaseMainCollection.h" +#include "RimSummaryEnsemble.h" + +#include "cafPdmPointer.h" + +#include +#include +#include + +namespace +{ +RimSummaryCase* createMockCase( int realizationNumber ) +{ + auto* summaryCase = new RimMockSummaryCase(); + + auto parameters = std::make_shared(); + parameters->setRealizationNumber( realizationNumber ); + summaryCase->setCaseRealizationParameters( parameters ); + + return summaryCase; +} +} // namespace + +//-------------------------------------------------------------------------------------------------- +/// Removing a source case makes a delta ensemble recreate and delete derived cases. The derived cases +/// are part of the list of cases to remove, and must not be left as dangling pointers in that list. +//-------------------------------------------------------------------------------------------------- +TEST( RimSummaryCaseMainCollection, RemoveCasesReferredByDeltaEnsemble ) +{ + RimSummaryCaseMainCollection* mainCollection = RiaSummaryTools::summaryCaseMainCollection(); + + auto* ensemble1 = mainCollection->addEnsemble( { createMockCase( 0 ), createMockCase( 1 ) }, "Ensemble 1", true ); + auto* ensemble2 = mainCollection->addEnsemble( { createMockCase( 0 ), createMockCase( 1 ) }, "Ensemble 2", true ); + + auto* deltaEnsemble = new RimDeltaSummaryEnsemble(); + mainCollection->addEnsemble( deltaEnsemble ); + deltaEnsemble->setEnsemble1( ensemble1 ); + deltaEnsemble->setEnsemble2( ensemble2 ); + deltaEnsemble->createDerivedEnsembleCases(); + + EXPECT_EQ( size_t( 2 ), deltaEnsemble->allSummaryCases().size() ); + + auto cases = mainCollection->allSummaryCases(); + EXPECT_EQ( size_t( 6 ), cases.size() ); + + // Guarded pointers are set to null when the object is deleted + std::vector> guardedCases( cases.begin(), cases.end() ); + + mainCollection->removeCases( cases ); + + size_t aliveCount = 0; + for ( const auto& guardedCase : guardedCases ) + { + if ( guardedCase.notNull() ) aliveCount++; + } + + // The two derived cases are deleted by the delta ensemble during removal + EXPECT_EQ( size_t( 4 ), aliveCount ); + EXPECT_EQ( aliveCount, cases.size() ); + + // No deleted case is left behind in the list + for ( auto* summaryCase : cases ) + { + auto isAlive = [summaryCase]( const caf::PdmPointer& guardedCase ) + { return guardedCase.notNull() && guardedCase.p() == summaryCase; }; + EXPECT_TRUE( std::any_of( guardedCases.begin(), guardedCases.end(), isAlive ) ); + } + + for ( auto* summaryCase : cases ) + { + delete summaryCase; + } + + mainCollection->removeEnsemble( deltaEnsemble ); + mainCollection->removeEnsemble( ensemble1 ); + mainCollection->removeEnsemble( ensemble2 ); + delete deltaEnsemble; + delete ensemble1; + delete ensemble2; +} From 1f6f08a3ddfe64c69a2e37ece8f16a5907186565 Mon Sep 17 00:00:00 2001 From: Magne Sjaastad Date: Thu, 13 Aug 2026 08:44:32 +0200 Subject: [PATCH 2/6] #14423 Add cycle-safe delta ensemble dependency traversal Delta ensembles were located by scanning the summary case main collection for objects referring to a given ensemble, and the dependent ensembles were visited by unguarded recursion. A dependency cycle, which is constructible through the UI, made that recursion run forever, and an ensemble used as both source 1 and source 2 was reported twice. Add dependentDeltaEnsembles(), deltaEnsemblesInUpdateOrder() and wouldCreateDependencyCycle() to RimSummaryEnsembleTools. The traversal uses the PDM back references, deduplicates, is iterative with visited and on-path sets, and returns the delta ensembles in topological order so a delta ensemble is always visited before the delta ensembles using it as a source. Back edges are logged instead of traversed. Reimplement updateDependentDeltaEnsembles on top of the new traversal and replace RimDeltaSummaryEnsemble::findReferringEnsembles() with dependentDeltaEnsembles() at its four call sites. Back references also find a delta ensemble that is detached from the project tree, which the previous ancestor scan did not. --- .../Summary/RimDeltaSummaryEnsemble.cpp | 34 +--- .../Summary/RimDeltaSummaryEnsemble.h | 2 - .../Summary/RimSummaryEnsembleTools.cpp | 127 +++++++++++- .../Summary/RimSummaryEnsembleTools.h | 12 ++ ApplicationLibCode/UnitTests/CMakeLists.txt | 1 + .../RimDeltaSummaryEnsemble-Test.cpp | 184 ++++++++++++++++++ 6 files changed, 325 insertions(+), 35 deletions(-) create mode 100644 ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp index 25004b37b0b..a01bd4cfc25 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp @@ -29,6 +29,7 @@ #include "RimProject.h" #include "RimSummaryCaseMainCollection.h" #include "RimSummaryEnsemble.h" +#include "RimSummaryEnsembleTools.h" #include "cafPdmUiButton.h" #include "cafPdmUiCheckBoxEditor.h" @@ -207,7 +208,7 @@ void RimDeltaSummaryEnsemble::createDerivedEnsembleCases() } // If other derived ensembles are referring to this ensemble, update their cases as well - for ( auto referring : findReferringEnsembles() ) + for ( auto referring : RimSummaryEnsembleTools::dependentDeltaEnsembles( this ) ) { referring->createDerivedEnsembleCases(); } @@ -404,7 +405,7 @@ void RimDeltaSummaryEnsemble::fieldChangedByUi( const caf::PdmFieldHandle* chang updateReferringCurveSetsZoomAll(); // If other derived ensembles are referring to this ensemble, update their cases as well - for ( auto referring : findReferringEnsembles() ) + for ( auto referring : RimSummaryEnsembleTools::dependentDeltaEnsembles( this ) ) { referring->updateReferringCurveSetsZoomAll(); } @@ -512,7 +513,7 @@ void RimDeltaSummaryEnsemble::updateDerivedEnsembleCases() } // If other derived ensembles are referring to this ensemble, update their cases as well - for ( auto referring : findReferringEnsembles() ) + for ( auto referring : RimSummaryEnsembleTools::dependentDeltaEnsembles( this ) ) { referring->updateDerivedEnsembleCases(); } @@ -552,31 +553,6 @@ RimSummaryCase* RimDeltaSummaryEnsemble::findCaseByRealizationNumber( const std: return nullptr; } -//-------------------------------------------------------------------------------------------------- -/// -//-------------------------------------------------------------------------------------------------- -std::vector RimDeltaSummaryEnsemble::findReferringEnsembles() const -{ - std::vector referringEnsembles; - - auto mainColl = firstAncestorOrThisOfType(); - if ( mainColl ) - { - for ( auto ensemble : mainColl->summaryEnsembles() ) - { - auto derivedEnsemble = dynamic_cast( ensemble ); - if ( derivedEnsemble ) - { - if ( derivedEnsemble->m_ensemble1() == this || derivedEnsemble->m_ensemble2() == this ) - { - referringEnsembles.push_back( derivedEnsemble ); - } - } - } - } - return referringEnsembles; -} - //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- @@ -614,7 +590,7 @@ void RimDeltaSummaryEnsemble::onSwapEnsemblesButtonClicked() updateConnectedEditors(); updateReferringCurveSetsZoomAll(); - for ( auto referring : findReferringEnsembles() ) + for ( auto referring : RimSummaryEnsembleTools::dependentDeltaEnsembles( this ) ) { referring->updateReferringCurveSetsZoomAll(); } diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.h b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.h index f540565e61f..16546a14fec 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.h +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.h @@ -87,8 +87,6 @@ class RimDeltaSummaryEnsemble : public RimSummaryEnsemble static RimSummaryCase* findCaseByParametersHash( const std::vector& cases, size_t hash ); static RimSummaryCase* findCaseByRealizationNumber( const std::vector& cases, int realizationNumber ); - std::vector findReferringEnsembles() const; - std::vector allEnsembles() const; private: diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsembleTools.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsembleTools.cpp index 53b1b0bd974..003d49b4178 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsembleTools.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsembleTools.cpp @@ -18,6 +18,8 @@ #include "RimSummaryEnsembleTools.h" +#include "RiaLogging.h" + #include "Summary/RiaSummaryTools.h" #include "RifReaderRftInterface.h" @@ -42,6 +44,8 @@ #include "cafPdmUiTreeView.h" +#include + //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- @@ -528,6 +532,123 @@ RimSummaryCase* RimSummaryEnsembleTools::caseWithMostDataObjects( const std::vec return nullptr; } +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +std::vector RimSummaryEnsembleTools::dependentDeltaEnsembles( const RimSummaryEnsemble* sourceEnsemble ) +{ + std::vector dependents; + if ( !sourceEnsemble ) return dependents; + + // One entry is returned per referring field, so a delta ensemble using the same ensemble as both sources is + // reported twice. Report each delta ensemble once. + std::set seen; + for ( auto deltaEnsemble : sourceEnsemble->objectsWithReferringPtrFieldsOfType() ) + { + if ( !deltaEnsemble ) continue; + if ( seen.insert( deltaEnsemble ).second ) dependents.push_back( deltaEnsemble ); + } + + return dependents; +} + +namespace +{ +//-------------------------------------------------------------------------------------------------- +/// Depth first traversal of the dependency graph, following the edges from a source ensemble to the delta ensembles +/// referring to it. The reverse postorder of that traversal is a topological order, so a delta ensemble is always +/// visited before the delta ensembles using it as a source. +//-------------------------------------------------------------------------------------------------- +std::vector dependentDeltaEnsemblesInOrder( const std::vector& sourceEnsembles ) +{ + struct StackEntry + { + RimDeltaSummaryEnsemble* ensemble = nullptr; + std::vector dependents; + size_t nextDependent = 0; + }; + + std::vector postOrder; + std::set visited; + std::set onPath; + std::vector stack; + + auto pushEnsemble = [&]( RimDeltaSummaryEnsemble* deltaEnsemble ) + { + if ( visited.count( deltaEnsemble ) ) return; + + if ( onPath.count( deltaEnsemble ) ) + { + RiaLogging::error( QString( "Delta ensemble '%1' is part of a circular dependency. The cycle is not traversed." ) + .arg( deltaEnsemble->name() ) + .toStdString() ); + return; + } + + onPath.insert( deltaEnsemble ); + stack.push_back( { deltaEnsemble, RimSummaryEnsembleTools::dependentDeltaEnsembles( deltaEnsemble ), 0 } ); + }; + + for ( auto sourceEnsemble : sourceEnsembles ) + { + for ( auto deltaEnsemble : RimSummaryEnsembleTools::dependentDeltaEnsembles( sourceEnsemble ) ) + { + pushEnsemble( deltaEnsemble ); + + while ( !stack.empty() ) + { + auto& top = stack.back(); + if ( top.nextDependent < top.dependents.size() ) + { + auto* dependent = top.dependents[top.nextDependent++]; + pushEnsemble( dependent ); + } + else + { + auto* completed = top.ensemble; + stack.pop_back(); + + onPath.erase( completed ); + visited.insert( completed ); + postOrder.push_back( completed ); + } + } + } + } + + std::reverse( postOrder.begin(), postOrder.end() ); + + return postOrder; +} +} // namespace + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +std::vector + RimSummaryEnsembleTools::deltaEnsemblesInUpdateOrder( const std::vector& sourceEnsembles ) +{ + return dependentDeltaEnsemblesInOrder( std::vector( sourceEnsembles.begin(), sourceEnsembles.end() ) ); +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +bool RimSummaryEnsembleTools::wouldCreateDependencyCycle( const RimDeltaSummaryEnsemble* deltaEnsemble, + const RimSummaryEnsemble* candidateSource ) +{ + if ( !deltaEnsemble || !candidateSource ) return false; + if ( deltaEnsemble == candidateSource ) return true; + + // Everything depending on deltaEnsemble, directly or indirectly, would close a cycle if used as a source for it + for ( auto dependent : dependentDeltaEnsemblesInOrder( { deltaEnsemble } ) ) + { + if ( dependent == candidateSource ) return true; + } + + return false; +} + //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- @@ -535,10 +656,8 @@ void RimSummaryEnsembleTools::updateDependentDeltaEnsembles( const RimSummaryEns { if ( !sourceEnsemble ) return; - auto referringObjects = sourceEnsemble->objectsWithReferringPtrFieldsOfType(); - for ( auto referringEnsemble : referringObjects ) + for ( auto deltaEnsemble : dependentDeltaEnsemblesInOrder( { sourceEnsemble } ) ) { - referringEnsemble->onSourceEnsembleChanged(); - updateDependentDeltaEnsembles( referringEnsemble ); + deltaEnsemble->onSourceEnsembleChanged(); } } \ No newline at end of file diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsembleTools.h b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsembleTools.h index cf1ab65d8e9..2e5297f5167 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsembleTools.h +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsembleTools.h @@ -27,6 +27,7 @@ class RimSummaryCase; class RimPlotCurve; class RigEnsembleParameter; class RimSummaryEnsemble; +class RimDeltaSummaryEnsemble; namespace RimSummaryEnsembleTools { @@ -47,6 +48,17 @@ void resetHighlightAllPlots(); RimSummaryCase* caseWithMostDataObjects( const std::vector& sourceCases ); +// The delta ensembles referring to sourceEnsemble, one entry per delta ensemble. An ensemble used as both source 1 and +// source 2 is reported once. +std::vector dependentDeltaEnsembles( const RimSummaryEnsemble* sourceEnsemble ); + +// All delta ensembles depending directly or indirectly on any of sourceEnsembles, ordered so that a delta ensemble +// always appears before the delta ensembles using it as a source. Cycles are reported and not traversed. +std::vector deltaEnsemblesInUpdateOrder( const std::vector& sourceEnsembles ); + +// True if using candidateSource as a source for deltaEnsemble would make the dependency graph cyclic. +bool wouldCreateDependencyCycle( const RimDeltaSummaryEnsemble* deltaEnsemble, const RimSummaryEnsemble* candidateSource ); + void updateDependentDeltaEnsembles( const RimSummaryEnsemble* sourceEnsemble ); } // namespace RimSummaryEnsembleTools diff --git a/ApplicationLibCode/UnitTests/CMakeLists.txt b/ApplicationLibCode/UnitTests/CMakeLists.txt index 8ac5af0cf61..922bab36c9e 100644 --- a/ApplicationLibCode/UnitTests/CMakeLists.txt +++ b/ApplicationLibCode/UnitTests/CMakeLists.txt @@ -86,6 +86,7 @@ set(SOURCE_UNITTEST_FILES ${CMAKE_CURRENT_LIST_DIR}/RimDataFilterCollection-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RimSummaryCaseCollection-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RimSummaryCaseMainCollection-Test.cpp + ${CMAKE_CURRENT_LIST_DIR}/RimDeltaSummaryEnsemble-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RifActiveCellsReader-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RifCsvDataTableFormatter-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RiaSummaryAddressAnalyzer-Test.cpp diff --git a/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp b/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp new file mode 100644 index 00000000000..7a8386f32bc --- /dev/null +++ b/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp @@ -0,0 +1,184 @@ +#include "gtest/gtest.h" + +#include "Summary/RiaSummaryTools.h" + +#include "RigCaseRealizationParameters.h" + +#include "RimDeltaSummaryEnsemble.h" +#include "RimMockSummaryCase.h" +#include "RimSummaryCaseMainCollection.h" +#include "RimSummaryEnsemble.h" +#include "RimSummaryEnsembleTools.h" + +#include "cafPdmPtrField.h" + +#include +#include +#include + +namespace +{ +RimSummaryCase* createMockCase( int realizationNumber ) +{ + auto* summaryCase = new RimMockSummaryCase(); + + auto parameters = std::make_shared(); + parameters->setRealizationNumber( realizationNumber ); + summaryCase->setCaseRealizationParameters( parameters ); + + return summaryCase; +} + +size_t countOf( const std::vector& ensembles, const RimDeltaSummaryEnsemble* ensemble ) +{ + return static_cast( std::count( ensembles.begin(), ensembles.end(), ensemble ) ); +} + +//-------------------------------------------------------------------------------------------------- +/// Assign a source ensemble without going through setEnsemble1()/setEnsemble2(). The setters trigger +/// an ensemble name update, and auto generated names never converge for a cyclic dependency. +//-------------------------------------------------------------------------------------------------- +void forceSourceEnsemble( RimDeltaSummaryEnsemble* deltaEnsemble, const QString& fieldKeyword, RimSummaryEnsemble* sourceEnsemble ) +{ + auto* field = dynamic_cast*>( deltaEnsemble->findField( fieldKeyword ) ); + ASSERT_TRUE( field != nullptr ); + + field->setValue( sourceEnsemble ); +} +} // namespace + +//-------------------------------------------------------------------------------------------------- +/// The summary case main collection is a shared global object, so every test must leave it empty to +/// keep the tests order independent. +//-------------------------------------------------------------------------------------------------- +class RimDeltaSummaryEnsembleTest : public ::testing::Test +{ +protected: + RimSummaryCaseMainCollection* mainCollection() const { return RiaSummaryTools::summaryCaseMainCollection(); } + + RimSummaryEnsemble* createEnsemble( const QString& name, const std::vector& realizationNumbers ) const + { + std::vector cases; + for ( auto realizationNumber : realizationNumbers ) + { + cases.push_back( createMockCase( realizationNumber ) ); + } + + return mainCollection()->addEnsemble( cases, name, true ); + } + + RimDeltaSummaryEnsemble* createDeltaEnsemble( RimSummaryEnsemble* ensemble1, RimSummaryEnsemble* ensemble2 ) const + { + auto* deltaEnsemble = new RimDeltaSummaryEnsemble(); + mainCollection()->addEnsemble( deltaEnsemble ); + deltaEnsemble->setEnsemble1( ensemble1 ); + deltaEnsemble->setEnsemble2( ensemble2 ); + + return deltaEnsemble; + } + + void TearDown() override + { + auto ensembles = mainCollection()->summaryEnsembles(); + + // Delete the delta ensembles first, they refer to the other ensembles + std::stable_partition( ensembles.begin(), + ensembles.end(), + []( RimSummaryEnsemble* ensemble ) { return dynamic_cast( ensemble ) != nullptr; } ); + + for ( auto* ensemble : ensembles ) + { + mainCollection()->removeEnsemble( ensemble ); + delete ensemble; + } + + for ( auto* summaryCase : mainCollection()->topLevelSummaryCases() ) + { + mainCollection()->removeCase( summaryCase, false ); + delete summaryCase; + } + } +}; + +//-------------------------------------------------------------------------------------------------- +/// objectsWithReferringPtrFields() returns one entry per referring field, so an ensemble used as both +/// sources of the same delta ensemble is reported twice by the raw PDM call. +//-------------------------------------------------------------------------------------------------- +TEST_F( RimDeltaSummaryEnsembleTest, DependentDeltaEnsembles_Deduplicated ) +{ + auto* ensemble = createEnsemble( "Ensemble", { 0, 1 } ); + auto* deltaEnsemble = createDeltaEnsemble( ensemble, ensemble ); + + EXPECT_EQ( size_t( 2 ), ensemble->objectsWithReferringPtrFieldsOfType().size() ); + + auto dependents = RimSummaryEnsembleTools::dependentDeltaEnsembles( ensemble ); + ASSERT_EQ( size_t( 1 ), dependents.size() ); + EXPECT_EQ( deltaEnsemble, dependents.front() ); +} + +//-------------------------------------------------------------------------------------------------- +/// A delta ensemble must be regenerated before the delta ensembles using it as a source. +//-------------------------------------------------------------------------------------------------- +TEST_F( RimDeltaSummaryEnsembleTest, DependencyOrder_ChainedDeltaEnsembles ) +{ + auto* ensemble1 = createEnsemble( "Ensemble 1", { 0, 1 } ); + auto* ensemble2 = createEnsemble( "Ensemble 2", { 0, 1 } ); + auto* ensemble3 = createEnsemble( "Ensemble 3", { 0, 1 } ); + + auto* deltaA = createDeltaEnsemble( ensemble1, ensemble2 ); + auto* deltaB = createDeltaEnsemble( deltaA, ensemble3 ); + + auto orderFromEnsemble1 = RimSummaryEnsembleTools::deltaEnsemblesInUpdateOrder( { ensemble1 } ); + ASSERT_EQ( size_t( 2 ), orderFromEnsemble1.size() ); + EXPECT_EQ( deltaA, orderFromEnsemble1[0] ); + EXPECT_EQ( deltaB, orderFromEnsemble1[1] ); + + // Ensemble 3 is only a source of the second delta ensemble + auto orderFromEnsemble3 = RimSummaryEnsembleTools::deltaEnsemblesInUpdateOrder( { ensemble3 } ); + ASSERT_EQ( size_t( 1 ), orderFromEnsemble3.size() ); + EXPECT_EQ( deltaB, orderFromEnsemble3[0] ); +} + +//-------------------------------------------------------------------------------------------------- +/// A cycle is constructible through the UI. The traversal must terminate and report each delta +/// ensemble once. +//-------------------------------------------------------------------------------------------------- +TEST_F( RimDeltaSummaryEnsembleTest, DependencyOrder_CycleTerminates ) +{ + auto* ensemble1 = createEnsemble( "Ensemble 1", { 0, 1 } ); + auto* ensemble2 = createEnsemble( "Ensemble 2", { 0, 1 } ); + + auto* deltaA = createDeltaEnsemble( ensemble1, ensemble2 ); + auto* deltaB = createDeltaEnsemble( deltaA, ensemble2 ); + + // Close the cycle, A now refers to B and B refers to A + forceSourceEnsemble( deltaA, "Ensemble1", deltaB ); + + auto order = RimSummaryEnsembleTools::deltaEnsemblesInUpdateOrder( { ensemble2 } ); + ASSERT_EQ( size_t( 2 ), order.size() ); + EXPECT_EQ( size_t( 1 ), countOf( order, deltaA ) ); + EXPECT_EQ( size_t( 1 ), countOf( order, deltaB ) ); + + EXPECT_TRUE( RimSummaryEnsembleTools::wouldCreateDependencyCycle( deltaB, deltaA ) ); + + // Break the cycle before tear down + forceSourceEnsemble( deltaA, "Ensemble1", ensemble1 ); +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +TEST_F( RimDeltaSummaryEnsembleTest, WouldCreateDependencyCycle ) +{ + auto* ensemble1 = createEnsemble( "Ensemble 1", { 0, 1 } ); + auto* ensemble2 = createEnsemble( "Ensemble 2", { 0, 1 } ); + + auto* deltaA = createDeltaEnsemble( ensemble1, ensemble2 ); + auto* deltaB = createDeltaEnsemble( deltaA, ensemble2 ); + + EXPECT_TRUE( RimSummaryEnsembleTools::wouldCreateDependencyCycle( deltaA, deltaA ) ); + EXPECT_TRUE( RimSummaryEnsembleTools::wouldCreateDependencyCycle( deltaA, deltaB ) ); + + EXPECT_FALSE( RimSummaryEnsembleTools::wouldCreateDependencyCycle( deltaB, deltaA ) ); + EXPECT_FALSE( RimSummaryEnsembleTools::wouldCreateDependencyCycle( deltaA, ensemble1 ) ); +} From b5bbdad75c38984745cd1242a6a2008292b75b5b Mon Sep 17 00:00:00 2001 From: Magne Sjaastad Date: Thu, 13 Aug 2026 08:52:10 +0200 Subject: [PATCH 3/6] #14423 Rebuild delta ensemble derived cases declaratively Derived cases were pooled through an m_inUse flag on RimDeltaSummaryCase. Every regeneration marked all cases not in use, which also severed their source references and cleared their caches, then handed them back out one by one and deleted whatever was left over. The flag conflated pool bookkeeping with owning the source references, allSummaryCases() hid the not-in-use cases from the rest of the project, and cases taken from the pool were pushed straight into m_cases without connecting nameChanged, so a renamed source case never propagated to the derived case. Replace the pooling with desiredSourceCasePairs(), a pure computation of the source case pairs the ensemble should have, and rebuildDerivedCases(), which diffs that against the existing derived cases keyed on the source case pointer pair. Matching cases are reused, missing ones are created, and surplus ones are detached and returned to the caller instead of being deleted in place. Keying on the pointer pair makes the rebuild idempotent, also right after project load where the derived cases arrive from XML with their sources already resolved. Add the protected RimSummaryEnsemble::addCaseWithoutDependencyUpdate(), used both by addCase() and by the rebuild, so a derived case gets nameChanged connected without triggering the dependent-ensemble notification that the rebuild is already performing itself. Remove setAllCasesNotInUse(), firstCaseNotInUse(), deleteCasesNoInUse(), RimDeltaSummaryCase::setInUse()/isInUse() and the m_inUse field, and add clearSourceCases() for the one thing setInUse(false) was actually needed for. Old project files keep loading, unknown XML keywords are skipped. The activeOnly parameter of allDerivedCases() is gone and RimDeltaSummaryEnsemble no longer overrides allSummaryCases(). --- .../Summary/RimDeltaSummaryCase.cpp | 30 +-- .../Summary/RimDeltaSummaryCase.h | 5 +- .../Summary/RimDeltaSummaryEnsemble.cpp | 191 +++++++++--------- .../Summary/RimDeltaSummaryEnsemble.h | 13 +- .../Summary/RimSummaryEnsemble.cpp | 23 ++- .../Summary/RimSummaryEnsemble.h | 4 + .../RimDeltaSummaryEnsemble-Test.cpp | 74 +++++++ 7 files changed, 207 insertions(+), 133 deletions(-) diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryCase.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryCase.cpp index df74724bafb..22aaa86b0d9 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryCase.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryCase.cpp @@ -183,7 +183,6 @@ RimDeltaSummaryCase::RimDeltaSummaryCase() CAF_PDM_InitFieldNoDefault( &m_useFixedTimeStep, "UseFixedTimeStep", "Use Fixed Time Step" ); CAF_PDM_InitField( &m_fixedTimeStepIndex, "FixedTimeStepIndex", 0, "Time Step" ); - CAF_PDM_InitField( &m_inUse, "InUse", false, "In Use" ); m_fixedTimeStepIndex.uiCapability()->setUiEditorTypeName( caf::PdmUiTreeSelectionEditor::uiEditorTypeName() ); m_fixedTimeStepIndex.uiCapability()->setUiLabelPosition( caf::PdmUiItemInfo::LabelPosition::HIDDEN ); } @@ -191,33 +190,22 @@ RimDeltaSummaryCase::RimDeltaSummaryCase() //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- -void RimDeltaSummaryCase::setInUse( bool inUse ) +void RimDeltaSummaryCase::setSummaryCases( RimSummaryCase* sumCase1, RimSummaryCase* sumCase2 ) { - m_inUse = inUse; - - if ( !m_inUse ) - { - m_summaryCase1 = nullptr; - m_summaryCase2 = nullptr; - m_dataCache.clear(); - } -} + m_summaryCase1 = sumCase1; + m_summaryCase2 = sumCase2; -//-------------------------------------------------------------------------------------------------- -/// -//-------------------------------------------------------------------------------------------------- -bool RimDeltaSummaryCase::isInUse() const -{ - return m_inUse; + clearCache(); } //-------------------------------------------------------------------------------------------------- -/// +/// Sever the references to the source cases. Used when a derived case is detached from its ensemble, +/// so it does not keep the source cases alive in any way after the ensemble has moved on. //-------------------------------------------------------------------------------------------------- -void RimDeltaSummaryCase::setSummaryCases( RimSummaryCase* sumCase1, RimSummaryCase* sumCase2 ) +void RimDeltaSummaryCase::clearSourceCases() { - m_summaryCase1 = sumCase1; - m_summaryCase2 = sumCase2; + m_summaryCase1 = nullptr; + m_summaryCase2 = nullptr; clearCache(); } diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryCase.h b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryCase.h index 9e27fab0b5d..cbc53c02bcb 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryCase.h +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryCase.h @@ -56,9 +56,8 @@ class RimDeltaSummaryCase : public RimSummaryCase, public RifSummaryReaderInterf public: RimDeltaSummaryCase(); - void setInUse( bool inUse ); - bool isInUse() const; void setSummaryCases( RimSummaryCase* sumCase1, RimSummaryCase* sumCase2 ); + void clearSourceCases(); void setOperator( DerivedSummaryOperator oper ); void setFixedTimeSteps( int fixedTimeStepCase1, int fixedTimeStepCase2 ); @@ -111,8 +110,6 @@ class RimDeltaSummaryCase : public RimSummaryCase, public RifSummaryReaderInterf caf::PdmField> m_useFixedTimeStep; caf::PdmField m_fixedTimeStepIndex; - caf::PdmField m_inUse; - // Local cache considered mutable mutable std::map, std::vector>> m_dataCache; }; diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp index a01bd4cfc25..27e8557b2a9 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp @@ -121,17 +121,6 @@ void RimDeltaSummaryEnsemble::setEnsemble2( RimSummaryEnsemble* ensemble ) RiaSummaryTools::updateSummaryEnsembleNames(); } -//-------------------------------------------------------------------------------------------------- -/// -//-------------------------------------------------------------------------------------------------- -std::vector RimDeltaSummaryEnsemble::allSummaryCases() const -{ - std::vector cases; - for ( auto sumCase : allDerivedCases( true ) ) - cases.push_back( sumCase ); - return cases; -} - //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- @@ -164,56 +153,119 @@ void RimDeltaSummaryEnsemble::createDerivedEnsembleCases() { if ( !m_ensemble1 || !m_ensemble2 ) return; - setAllCasesNotInUse(); + auto orphanedCases = rebuildDerivedCases(); + + // If other derived ensembles are referring to this ensemble, update their cases as well + for ( auto referring : RimSummaryEnsembleTools::dependentDeltaEnsembles( this ) ) + { + referring->createDerivedEnsembleCases(); + } + + for ( auto orphanedCase : orphanedCases ) + { + delete orphanedCase; + } +} + +//-------------------------------------------------------------------------------------------------- +/// The source case pairs this ensemble should have derived cases for, in the order of the cases in +/// the first source ensemble. Pure computation, the object graph is not modified. +//-------------------------------------------------------------------------------------------------- +std::vector> RimDeltaSummaryEnsemble::desiredSourceCasePairs() const +{ + std::vector> casePairs; + + if ( !m_ensemble1 || !m_ensemble2 ) return casePairs; const auto cases1 = m_ensemble1->allSummaryCases(); const auto cases2 = m_ensemble2->allSummaryCases(); - for ( auto& sumCase1 : cases1 ) + for ( auto sumCase1 : cases1 ) { auto crp = sumCase1->caseRealizationParameters(); if ( !crp ) continue; - RimSummaryCase* summaryCase2 = nullptr; - if ( m_matchOnParameters ) - { - summaryCase2 = findCaseByParametersHash( cases2, crp->parametersHash() ); - } - else - { - summaryCase2 = findCaseByRealizationNumber( cases2, crp->realizationNumber() ); - } + RimSummaryCase* summaryCase2 = m_matchOnParameters ? findCaseByParametersHash( cases2, crp->parametersHash() ) + : findCaseByRealizationNumber( cases2, crp->realizationNumber() ); if ( !summaryCase2 ) continue; - auto derivedCase = firstCaseNotInUse(); - derivedCase->setSummaryCases( sumCase1, summaryCase2 ); - derivedCase->setOperator( m_operator() ); + casePairs.emplace_back( sumCase1, summaryCase2 ); + } + + return casePairs; +} + +//-------------------------------------------------------------------------------------------------- +/// Bring the derived cases in sync with desiredSourceCasePairs(). Existing derived cases are matched +/// on the source case pair, so a rebuild producing the same pairs reuses the same objects. +/// +/// Surplus cases are detached and returned, never deleted. Destroying them is the responsibility of +/// the caller, which may be several frames above a caller still iterating over these very objects. +//-------------------------------------------------------------------------------------------------- +std::vector RimDeltaSummaryEnsemble::rebuildDerivedCases() +{ + std::vector orphanedCases; + + std::map, RimDeltaSummaryCase*> reusableCases; + for ( auto derivedCase : allDerivedCases() ) + { + auto sourceCase1 = derivedCase->summaryCase1(); + auto sourceCase2 = derivedCase->summaryCase2(); + + // A case with unresolved sources, or a duplicate of a pair already seen, can not be reused + if ( sourceCase1 && sourceCase2 && reusableCases.try_emplace( { sourceCase1, sourceCase2 }, derivedCase ).second ) continue; - int fixedTimeStepCase1 = -1; - int fixedTimeStepCase2 = -1; - if ( m_useFixedTimeStep == FixedTimeStepMode::FIXED_TIME_STEP_CASE_1 ) + orphanedCases.push_back( derivedCase ); + } + + int fixedTimeStepCase1 = -1; + int fixedTimeStepCase2 = -1; + if ( m_useFixedTimeStep == FixedTimeStepMode::FIXED_TIME_STEP_CASE_1 ) + { + fixedTimeStepCase1 = m_fixedTimeStepIndex; + } + else if ( m_useFixedTimeStep == FixedTimeStepMode::FIXED_TIME_STEP_CASE_2 ) + { + fixedTimeStepCase2 = m_fixedTimeStepIndex; + } + + for ( const auto& [sourceCase1, sourceCase2] : desiredSourceCasePairs() ) + { + RimDeltaSummaryCase* derivedCase = nullptr; + + auto it = reusableCases.find( { sourceCase1, sourceCase2 } ); + if ( it != reusableCases.end() ) { - fixedTimeStepCase1 = m_fixedTimeStepIndex; + derivedCase = it->second; + reusableCases.erase( it ); } - else if ( m_useFixedTimeStep == FixedTimeStepMode::FIXED_TIME_STEP_CASE_2 ) + else { - fixedTimeStepCase2 = m_fixedTimeStepIndex; + derivedCase = new RimDeltaSummaryCase(); + addCaseWithoutDependencyUpdate( derivedCase ); + derivedCase->setSummaryCases( sourceCase1, sourceCase2 ); } + derivedCase->setOperator( m_operator() ); derivedCase->setFixedTimeSteps( fixedTimeStepCase1, fixedTimeStepCase2 ); derivedCase->createSummaryReaderInterface(); - derivedCase->setCaseRealizationParameters( crp ); - derivedCase->setInUse( true ); + derivedCase->setCaseRealizationParameters( sourceCase1->caseRealizationParameters() ); derivedCase->updateDisplayNameFromCases(); } - // If other derived ensembles are referring to this ensemble, update their cases as well - for ( auto referring : RimSummaryEnsembleTools::dependentDeltaEnsembles( this ) ) + // The reusable cases not claimed by a desired pair are surplus + for ( const auto& [casePair, derivedCase] : reusableCases ) { - referring->createDerivedEnsembleCases(); + orphanedCases.push_back( derivedCase ); + } + + for ( auto orphanedCase : orphanedCases ) + { + removeCase( orphanedCase, false ); + orphanedCase->clearSourceCases(); } - deleteCasesNoInUse(); + return orphanedCases; } //-------------------------------------------------------------------------------------------------- @@ -431,68 +483,17 @@ void RimDeltaSummaryEnsemble::defineEditorAttribute( const caf::PdmFieldHandle* //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- -void RimDeltaSummaryEnsemble::setAllCasesNotInUse() -{ - for ( auto derCase : allDerivedCases( true ) ) - derCase->setInUse( false ); -} - -//-------------------------------------------------------------------------------------------------- -/// -//-------------------------------------------------------------------------------------------------- -void RimDeltaSummaryEnsemble::deleteCasesNoInUse() -{ - std::vector inactiveCases; - auto allCases = allDerivedCases( false ); - std::copy_if( allCases.begin(), - allCases.end(), - std::back_inserter( inactiveCases ), - []( RimDeltaSummaryCase* derCase ) { return !derCase->isInUse(); } ); - - for ( auto derCase : inactiveCases ) - { - removeCase( derCase ); - delete derCase; - } -} - -//-------------------------------------------------------------------------------------------------- -/// -//-------------------------------------------------------------------------------------------------- -RimDeltaSummaryCase* RimDeltaSummaryEnsemble::firstCaseNotInUse() -{ - auto allCases = allDerivedCases( false ); - auto itr = std::find_if( allCases.begin(), allCases.end(), []( RimDeltaSummaryCase* derCase ) { return !derCase->isInUse(); } ); - if ( itr != allCases.end() ) - { - return *itr; - } - - // If no active case was found, add a new case to the collection - auto newCase = new RimDeltaSummaryCase(); - - // Show realization data source for the first case. If we create for all, the performance will be bad - newCase->setShowTreeNodes( m_cases.empty() ); - - m_cases.push_back( newCase ); - return newCase; -} - -//-------------------------------------------------------------------------------------------------- -/// -//-------------------------------------------------------------------------------------------------- -std::vector RimDeltaSummaryEnsemble::allDerivedCases( bool activeOnly ) const +std::vector RimDeltaSummaryEnsemble::allDerivedCases() const { - std::vector activeCases; - for ( auto sumCase : RimSummaryEnsemble::allSummaryCases() ) + std::vector derivedCases; + for ( auto sumCase : allSummaryCases() ) { - auto derivedCase = dynamic_cast( sumCase ); - if ( derivedCase && ( !activeOnly || derivedCase->isInUse() ) ) + if ( auto derivedCase = dynamic_cast( sumCase ) ) { - activeCases.push_back( derivedCase ); + derivedCases.push_back( derivedCase ); } } - return activeCases; + return derivedCases; } //-------------------------------------------------------------------------------------------------- @@ -500,7 +501,7 @@ std::vector RimDeltaSummaryEnsemble::allDerivedCases( bool //-------------------------------------------------------------------------------------------------- void RimDeltaSummaryEnsemble::updateDerivedEnsembleCases() { - for ( auto& derivedCase : allDerivedCases( true ) ) + for ( auto& derivedCase : allDerivedCases() ) { derivedCase->createSummaryReaderInterface(); diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.h b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.h index 16546a14fec..f7fdf70f4db 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.h +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.h @@ -53,7 +53,6 @@ class RimDeltaSummaryEnsemble : public RimSummaryEnsemble void setEnsemble1( RimSummaryEnsemble* ensemble ); void setEnsemble2( RimSummaryEnsemble* ensemble ); - std::vector allSummaryCases() const override; std::set ensembleSummaryAddresses() const override; bool hasCaseReference( const RimSummaryCase* sumCase ) const; @@ -63,6 +62,13 @@ class RimDeltaSummaryEnsemble : public RimSummaryEnsemble void onSourceEnsembleChanged(); void createDerivedEnsembleCases(); + std::vector> desiredSourceCasePairs() const; + + // Detaches and returns the derived cases no longer backed by a source case pair. The caller owns them. + [[nodiscard]] std::vector rebuildDerivedCases(); + + std::vector allDerivedCases() const; + bool discardMissingOrIncompleteRealizations() const; std::pair nameKeys() const override; @@ -76,11 +82,6 @@ class RimDeltaSummaryEnsemble : public RimSummaryEnsemble void onSwapEnsemblesButtonClicked(); - void setAllCasesNotInUse(); - void deleteCasesNoInUse(); - RimDeltaSummaryCase* firstCaseNotInUse(); - std::vector allDerivedCases( bool activeOnly ) const; - void updateDerivedEnsembleCases(); bool isValid() const; diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.cpp index b9c7ed443a7..ce59d075adc 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.cpp @@ -155,13 +155,7 @@ void RimSummaryEnsemble::removeCase( RimSummaryCase* summaryCase, bool notifyCha //-------------------------------------------------------------------------------------------------- void RimSummaryEnsemble::addCase( RimSummaryCase* summaryCase, bool notifyChange ) { - summaryCase->nameChanged.connect( this, &RimSummaryEnsemble::onCaseNameChanged ); - - summaryCase->setShowTreeNodes( m_cases.empty() ); - - m_cases.push_back( summaryCase ); - m_cachedSortedEnsembleParameters.clear(); - m_analyzer.reset(); + addCaseWithoutDependencyUpdate( summaryCase ); // Update derived ensemble cases (if any) std::vector referringObjects = objectsWithReferringPtrFieldsOfType(); @@ -180,6 +174,21 @@ void RimSummaryEnsemble::addCase( RimSummaryCase* summaryCase, bool notifyChange } if ( notifyChange ) updateReferringCurveSetsZoomAll(); +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +void RimSummaryEnsemble::addCaseWithoutDependencyUpdate( RimSummaryCase* summaryCase ) +{ + summaryCase->nameChanged.connect( this, &RimSummaryEnsemble::onCaseNameChanged ); + + // Show realization data source for the first case. If we create for all, the performance will be bad + summaryCase->setShowTreeNodes( m_cases.empty() ); + + m_cases.push_back( summaryCase ); + m_cachedSortedEnsembleParameters.clear(); + m_analyzer.reset(); clearChildNodes(); } diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.h b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.h index ce17ef3ad06..f610ea0b483 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.h +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryEnsemble.h @@ -128,6 +128,10 @@ class RimSummaryEnsemble : public caf::PdmObject protected: virtual void onLoadDataAndUpdate(); + // Add a case without notifying the delta ensembles depending on this ensemble. Used when the caller is itself the + // owner of the dependency update, as is the case when a delta ensemble rebuilds its derived cases. + void addCaseWithoutDependencyUpdate( RimSummaryCase* summaryCase ); + void defineUiOrdering( QString uiConfigName, caf::PdmUiOrdering& uiOrdering ) override; void buildMetaData(); void appendMenuItems( caf::CmdFeatureMenuBuilder& menuBuilder ) const override; diff --git a/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp b/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp index 7a8386f32bc..b6e55c84010 100644 --- a/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp +++ b/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp @@ -165,6 +165,80 @@ TEST_F( RimDeltaSummaryEnsembleTest, DependencyOrder_CycleTerminates ) forceSourceEnsemble( deltaA, "Ensemble1", ensemble1 ); } +//-------------------------------------------------------------------------------------------------- +/// Derived cases are matched on the source case pair, so rebuilding without changing the sources must +/// reuse the very same objects. +//-------------------------------------------------------------------------------------------------- +TEST_F( RimDeltaSummaryEnsembleTest, Rebuild_IsIdempotent ) +{ + auto* ensemble1 = createEnsemble( "Ensemble 1", { 0, 1 } ); + auto* ensemble2 = createEnsemble( "Ensemble 2", { 0, 1 } ); + + auto* deltaEnsemble = createDeltaEnsemble( ensemble1, ensemble2 ); + deltaEnsemble->createDerivedEnsembleCases(); + + auto derivedCases = deltaEnsemble->allDerivedCases(); + ASSERT_EQ( size_t( 2 ), derivedCases.size() ); + + auto orphanedCases = deltaEnsemble->rebuildDerivedCases(); + EXPECT_TRUE( orphanedCases.empty() ); + + EXPECT_EQ( derivedCases, deltaEnsemble->allDerivedCases() ); +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +TEST_F( RimDeltaSummaryEnsembleTest, Rebuild_CreatesMissingAndOrphansSurplus ) +{ + auto* ensemble1 = createEnsemble( "Ensemble 1", { 0, 1 } ); + auto* ensemble2 = createEnsemble( "Ensemble 2", { 0, 1 } ); + + auto* deltaEnsemble = createDeltaEnsemble( ensemble1, ensemble2 ); + deltaEnsemble->createDerivedEnsembleCases(); + ASSERT_EQ( size_t( 2 ), deltaEnsemble->allDerivedCases().size() ); + + // A matching realization in both source ensembles gives one more derived case + auto* addedCase1 = createMockCase( 2 ); + auto* addedCase2 = createMockCase( 2 ); + ensemble1->addCase( addedCase1, false ); + ensemble2->addCase( addedCase2, false ); + + auto orphanedCases = deltaEnsemble->rebuildDerivedCases(); + EXPECT_TRUE( orphanedCases.empty() ); + EXPECT_EQ( size_t( 3 ), deltaEnsemble->allDerivedCases().size() ); + + // Removing it again makes the derived case surplus + ensemble1->removeCase( addedCase1, false ); + delete addedCase1; + + orphanedCases = deltaEnsemble->rebuildDerivedCases(); + ASSERT_EQ( size_t( 1 ), orphanedCases.size() ); + EXPECT_EQ( size_t( 2 ), deltaEnsemble->allDerivedCases().size() ); + + for ( auto* orphanedCase : orphanedCases ) + { + delete orphanedCase; + } +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +TEST_F( RimDeltaSummaryEnsembleTest, Rebuild_NoMatchingRealizations ) +{ + auto* ensemble1 = createEnsemble( "Ensemble 1", { 0, 1 } ); + auto* ensemble2 = createEnsemble( "Ensemble 2", { 5, 6 } ); + + auto* deltaEnsemble = createDeltaEnsemble( ensemble1, ensemble2 ); + + EXPECT_TRUE( deltaEnsemble->desiredSourceCasePairs().empty() ); + + auto orphanedCases = deltaEnsemble->rebuildDerivedCases(); + EXPECT_TRUE( orphanedCases.empty() ); + EXPECT_TRUE( deltaEnsemble->allDerivedCases().empty() ); +} + //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- From 08a14d8823b98843606dec83f4838b680de77f6c Mon Sep 17 00:00:00 2001 From: Magne Sjaastad Date: Thu, 13 Aug 2026 09:21:16 +0200 Subject: [PATCH 4/6] #14423 Defer destruction of orphaned derived cases to a batch flush Closing all summary cases while a delta ensemble is present crashed with a use-after-free. RicCloseSummaryCaseFeature::deleteSummaryCases holds a case list across removeCases and deletes it afterwards, while removeCases made the delta ensemble rebuild and destroy derived cases that are themselves part of that list. The previous fix rewrote the caller list with the surviving cases, which stopped the crash but left the hazard in place for any other caller holding a case list across a removal. Add RimSummaryCaseUpdateBatch, a plain scope object that is ambient for the duration of its scope. Removal now only detaches, and hands the detached cases to the batch. The outermost scope flushes, regenerating the dirty delta ensembles in dependency order first so a chained delta ensemble sees the final state of its source, then destroying the orphans with caf::PdmObjectHandleTools::deleteObjects. A nested batch contributes to the outermost one and never flushes. Orphans are held as guarded pointers, so a case the caller destroyed itself is skipped instead of being destroyed twice. Both contribution points fall back to immediate execution when no batch is active, so call sites that do not open one keep behaving as before. Open a batch in RimSummaryCaseMainCollection::removeCases and in RicCloseSummaryCaseFeature::deleteSummaryCases, which is the outermost of the two and therefore keeps the detached cases alive across its own deleteObjects call. removeCases no longer rewrites the caller list, so it now takes it by const reference. --- .../Commands/RicCloseSummaryCaseFeature.cpp | 5 + .../Summary/CMakeLists_files.cmake | 1 + .../Summary/RimDeltaSummaryEnsemble.cpp | 6 +- .../Summary/RimSummaryCaseMainCollection.cpp | 21 +-- .../Summary/RimSummaryCaseMainCollection.h | 4 +- .../Summary/RimSummaryCaseUpdateBatch.cpp | 157 ++++++++++++++++++ .../Summary/RimSummaryCaseUpdateBatch.h | 68 ++++++++ .../RimDeltaSummaryEnsemble-Test.cpp | 65 ++++++++ .../RimSummaryCaseMainCollection-Test.cpp | 44 ++--- 9 files changed, 331 insertions(+), 40 deletions(-) create mode 100644 ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseUpdateBatch.cpp create mode 100644 ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseUpdateBatch.h diff --git a/ApplicationLibCode/Commands/RicCloseSummaryCaseFeature.cpp b/ApplicationLibCode/Commands/RicCloseSummaryCaseFeature.cpp index 3170bd1e9d3..f5ad23c3dc4 100644 --- a/ApplicationLibCode/Commands/RicCloseSummaryCaseFeature.cpp +++ b/ApplicationLibCode/Commands/RicCloseSummaryCaseFeature.cpp @@ -28,6 +28,7 @@ #include "RimProject.h" #include "RimSummaryCase.h" #include "RimSummaryCaseMainCollection.h" +#include "RimSummaryCaseUpdateBatch.h" #include "RimSummaryMultiPlot.h" #include "RimSummaryMultiPlotCollection.h" #include "RimSummaryPlot.h" @@ -61,6 +62,10 @@ void RicCloseSummaryCaseFeature::setupActionLook( QAction* actionToSetup ) //-------------------------------------------------------------------------------------------------- void RicCloseSummaryCaseFeature::deleteSummaryCases( std::vector cases ) { + // The case list is used all the way down to the delete below. Keep a batch open for that whole span, so a derived + // case detached by the removal is not destroyed while this list still refers to it. + RimSummaryCaseUpdateBatch updateBatch; + RimSummaryMultiPlotCollection* summaryPlotColl = RiaSummaryTools::summaryMultiPlotCollection(); RimSummaryCaseMainCollection* summaryCaseMainCollection = RiaSummaryTools::summaryCaseMainCollection(); diff --git a/ApplicationLibCode/ProjectDataModel/Summary/CMakeLists_files.cmake b/ApplicationLibCode/ProjectDataModel/Summary/CMakeLists_files.cmake index 7eb5a25eab3..5e0687122ad 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/CMakeLists_files.cmake +++ b/ApplicationLibCode/ProjectDataModel/Summary/CMakeLists_files.cmake @@ -55,6 +55,7 @@ set(SOURCE_GROUP_SOURCE_FILES ${CMAKE_CURRENT_LIST_DIR}/RimSummaryAddressSelector.cpp ${CMAKE_CURRENT_LIST_DIR}/RimEnsembleCrossPlotStatisticsCase.cpp ${CMAKE_CURRENT_LIST_DIR}/RimSummaryEnsembleTools.cpp + ${CMAKE_CURRENT_LIST_DIR}/RimSummaryCaseUpdateBatch.cpp ${CMAKE_CURRENT_LIST_DIR}/RimSummaryPlotReadOut.cpp ) diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp index 27e8557b2a9..38f311ad8a2 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimDeltaSummaryEnsemble.cpp @@ -28,6 +28,7 @@ #include "RimDeltaSummaryEnsemble.h" #include "RimProject.h" #include "RimSummaryCaseMainCollection.h" +#include "RimSummaryCaseUpdateBatch.h" #include "RimSummaryEnsemble.h" #include "RimSummaryEnsembleTools.h" @@ -161,10 +162,7 @@ void RimDeltaSummaryEnsemble::createDerivedEnsembleCases() referring->createDerivedEnsembleCases(); } - for ( auto orphanedCase : orphanedCases ) - { - delete orphanedCase; - } + RimSummaryCaseUpdateBatch::orphan( orphanedCases ); } //-------------------------------------------------------------------------------------------------- diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp index dc80e2e5fe8..77c1841ccae 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.cpp @@ -48,6 +48,7 @@ #include "RimProject.h" #include "RimRftPlotCollection.h" #include "RimSummaryCase.h" +#include "RimSummaryCaseUpdateBatch.h" #include "RimSummaryCurve.h" #include "RimSummaryEnsemble.h" #include "RimSummaryMultiPlotCollection.h" @@ -227,16 +228,16 @@ void RimSummaryCaseMainCollection::removeCase( RimSummaryCase* summaryCase, bool //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- -void RimSummaryCaseMainCollection::removeCases( std::vector& cases ) +void RimSummaryCaseMainCollection::removeCases( const std::vector& cases ) { - // Removing one case can delete other cases in the list. A delta ensemble recreates its derived cases when a source - // case is removed, and deletes the derived cases no longer in use. Use guarded pointers to avoid touching deleted - // cases, and return only the cases that are still alive. - std::vector> guardedCases( cases.begin(), cases.end() ); + // Removing one case makes a delta ensemble rebuild its derived cases, and some of those derived cases can be part + // of this very list. The batch detaches them now and destroys them when the outermost batch scope ends, so a caller + // holding on to the list across this call never sees a freed case, as long as it opens a batch of its own. + RimSummaryCaseUpdateBatch updateBatch; - for ( const auto& sumCase : guardedCases ) + for ( auto sumCase : cases ) { - if ( sumCase.notNull() ) removeCase( sumCase, false ); + removeCase( sumCase, false ); } for ( RimSummaryEnsemble* ensemble : m_ensembles ) @@ -244,12 +245,6 @@ void RimSummaryCaseMainCollection::removeCases( std::vector& ca ensemble->updateReferringCurveSetsZoomAll(); } - cases.clear(); - for ( const auto& sumCase : guardedCases ) - { - if ( sumCase.notNull() ) cases.push_back( sumCase ); - } - dataSourceHasChanged.send(); } diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.h b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.h index f5d480def0b..b3c152464d1 100644 --- a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.h +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseMainCollection.h @@ -60,8 +60,8 @@ class RimSummaryCaseMainCollection : public caf::PdmObject void addCases( const std::vector cases ); void addCase( RimSummaryCase* summaryCase ); void removeCase( RimSummaryCase* summaryCase, bool notifyChange = true ); - // Cases deleted as a side effect of the removal are erased from the input vector - void removeCases( std::vector& cases ); + // Open a RimSummaryCaseUpdateBatch around this call if the case list is used afterwards, see removeCases() + void removeCases( const std::vector& cases ); void moveCase( RimSummaryCase* summaryCase, int destinationIndex ); RimSummaryEnsemble* addEnsemble( const std::vector& summaryCases, diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseUpdateBatch.cpp b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseUpdateBatch.cpp new file mode 100644 index 00000000000..a5d2399dfe6 --- /dev/null +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseUpdateBatch.cpp @@ -0,0 +1,157 @@ +///////////////////////////////////////////////////////////////////////////////// +// +// Copyright (C) 2026- Equinor ASA +// +// ResInsight 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. +// +// ResInsight 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. +// +///////////////////////////////////////////////////////////////////////////////// + +#include "RimSummaryCaseUpdateBatch.h" + +#include "RimDeltaSummaryCase.h" +#include "RimDeltaSummaryEnsemble.h" +#include "RimSummaryEnsembleTools.h" + +#include "cafPdmObjectHandleTools.h" + +#include +#include + +RimSummaryCaseUpdateBatch* RimSummaryCaseUpdateBatch::sm_current = nullptr; + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +RimSummaryCaseUpdateBatch::RimSummaryCaseUpdateBatch() +{ + // A nested batch contributes to the outermost one and never flushes + if ( !sm_current ) sm_current = this; +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +RimSummaryCaseUpdateBatch::~RimSummaryCaseUpdateBatch() +{ + if ( sm_current != this ) return; + + // Clear the ambient batch before flushing. Work triggered by the flush itself is executed immediately, which is + // safe here, as the outermost scope is ending and no caller is holding a case list across this point. + sm_current = nullptr; + + flush(); +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +bool RimSummaryCaseUpdateBatch::isActive() +{ + return sm_current != nullptr; +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +void RimSummaryCaseUpdateBatch::orphan( const std::vector& orphanedCases ) +{ + if ( orphanedCases.empty() ) return; + + if ( sm_current ) + { + sm_current->m_orphanedCases.insert( sm_current->m_orphanedCases.end(), orphanedCases.begin(), orphanedCases.end() ); + return; + } + + auto casesToDelete = orphanedCases; + caf::PdmObjectHandleTools::deleteObjects( casesToDelete ); +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +void RimSummaryCaseUpdateBatch::markDeltaEnsembleDirty( RimDeltaSummaryEnsemble* deltaEnsemble ) +{ + if ( !deltaEnsemble ) return; + + if ( sm_current ) + { + auto& dirtyEnsembles = sm_current->m_dirtyEnsembles; + + auto isSameEnsemble = [deltaEnsemble]( const caf::PdmPointer& candidate ) + { return candidate.p() == deltaEnsemble; }; + + if ( std::none_of( dirtyEnsembles.begin(), dirtyEnsembles.end(), isSameEnsemble ) ) + { + dirtyEnsembles.push_back( deltaEnsemble ); + } + return; + } + + deltaEnsemble->onSourceEnsembleChanged(); +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +std::vector RimSummaryCaseUpdateBatch::ensemblesToRegenerate() const +{ + std::vector dirtyEnsembles; + for ( const auto& dirtyEnsemble : m_dirtyEnsembles ) + { + // An ensemble marked dirty can have been deleted before the flush + if ( dirtyEnsemble.notNull() ) dirtyEnsembles.push_back( dirtyEnsemble.p() ); + } + + // Everything depending on the dirty ensembles, in dependency order. A dirty ensemble depending on another dirty + // ensemble shows up here, and must be regenerated in this order rather than as a root. + auto dependents = RimSummaryEnsembleTools::deltaEnsemblesInUpdateOrder( dirtyEnsembles ); + + std::set alreadyOrdered( dependents.begin(), dependents.end() ); + + std::vector ordered; + for ( const auto& dirtyEnsemble : m_dirtyEnsembles ) + { + if ( dirtyEnsemble.isNull() ) continue; + if ( !alreadyOrdered.insert( dirtyEnsemble.p() ).second ) continue; + + ordered.push_back( dirtyEnsemble.p() ); + } + + ordered.insert( ordered.end(), dependents.begin(), dependents.end() ); + + return ordered; +} + +//-------------------------------------------------------------------------------------------------- +/// Regenerate before destroying, so a chained delta ensemble sees the final state of its source +/// before anything is freed. +//-------------------------------------------------------------------------------------------------- +void RimSummaryCaseUpdateBatch::flush() +{ + for ( auto deltaEnsemble : ensemblesToRegenerate() ) + { + deltaEnsemble->onSourceEnsembleChanged(); + } + m_dirtyEnsembles.clear(); + + std::vector casesToDelete; + for ( const auto& orphanedCase : m_orphanedCases ) + { + // An orphaned case can already have been destroyed by the code that detached it + if ( orphanedCase.notNull() ) casesToDelete.push_back( orphanedCase.p() ); + } + m_orphanedCases.clear(); + + caf::PdmObjectHandleTools::deleteObjects( casesToDelete ); +} diff --git a/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseUpdateBatch.h b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseUpdateBatch.h new file mode 100644 index 00000000000..ae597950583 --- /dev/null +++ b/ApplicationLibCode/ProjectDataModel/Summary/RimSummaryCaseUpdateBatch.h @@ -0,0 +1,68 @@ +///////////////////////////////////////////////////////////////////////////////// +// +// Copyright (C) 2026- Equinor ASA +// +// ResInsight 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. +// +// ResInsight 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. +// +///////////////////////////////////////////////////////////////////////////////// + +#pragma once + +#include "cafPdmPointer.h" + +#include + +class RimDeltaSummaryCase; +class RimDeltaSummaryEnsemble; + +//================================================================================================== +/// Collects the work that must happen once a set of summary case mutations has settled. Removing a +/// case only detaches it and hands it to the batch, destruction happens when the batch flushes. +/// +/// This is a plain stack object, ambient for the duration of its scope. A nested batch contributes to +/// the outermost one, which is the only one that flushes. Open a batch wherever a case list is held +/// across a mutation, so no object in that list is freed while the list is still in use. +/// +/// The contribution points fall back to immediate execution when no batch is active, so call sites +/// that do not open one keep behaving as before. +//================================================================================================== +class RimSummaryCaseUpdateBatch +{ +public: + RimSummaryCaseUpdateBatch(); + ~RimSummaryCaseUpdateBatch(); + + RimSummaryCaseUpdateBatch( const RimSummaryCaseUpdateBatch& ) = delete; + RimSummaryCaseUpdateBatch& operator=( const RimSummaryCaseUpdateBatch& ) = delete; + + // Hand over detached cases for destruction at the flush. Destroys them immediately if no batch is active. + static void orphan( const std::vector& orphanedCases ); + + // Schedule a regeneration of the derived cases of a delta ensemble. Regenerates immediately if no batch is active. + static void markDeltaEnsembleDirty( RimDeltaSummaryEnsemble* deltaEnsemble ); + + static bool isActive(); + +private: + void flush(); + + // The dirty ensembles and everything depending on them, ordered so a delta ensemble is regenerated before the delta + // ensembles using it as a source + std::vector ensemblesToRegenerate() const; + +private: + std::vector> m_orphanedCases; + std::vector> m_dirtyEnsembles; + + static RimSummaryCaseUpdateBatch* sm_current; +}; diff --git a/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp b/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp index b6e55c84010..774e1454c87 100644 --- a/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp +++ b/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp @@ -7,9 +7,11 @@ #include "RimDeltaSummaryEnsemble.h" #include "RimMockSummaryCase.h" #include "RimSummaryCaseMainCollection.h" +#include "RimSummaryCaseUpdateBatch.h" #include "RimSummaryEnsemble.h" #include "RimSummaryEnsembleTools.h" +#include "cafPdmPointer.h" #include "cafPdmPtrField.h" #include @@ -239,6 +241,69 @@ TEST_F( RimDeltaSummaryEnsembleTest, Rebuild_NoMatchingRealizations ) EXPECT_TRUE( deltaEnsemble->allDerivedCases().empty() ); } +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +TEST_F( RimDeltaSummaryEnsembleTest, Batch_DeletionDeferredToFlush ) +{ + auto* ensemble1 = createEnsemble( "Ensemble 1", { 0, 1 } ); + auto* ensemble2 = createEnsemble( "Ensemble 2", { 0, 1 } ); + + auto* deltaEnsemble = createDeltaEnsemble( ensemble1, ensemble2 ); + deltaEnsemble->createDerivedEnsembleCases(); + ASSERT_EQ( size_t( 2 ), deltaEnsemble->allDerivedCases().size() ); + + caf::PdmPointer guardedCase = deltaEnsemble->allDerivedCases().front(); + + { + RimSummaryCaseUpdateBatch updateBatch; + EXPECT_TRUE( RimSummaryCaseUpdateBatch::isActive() ); + + auto* sourceCase = ensemble1->allSummaryCases().front(); + mainCollection()->removeCase( sourceCase, false ); + delete sourceCase; + + // The derived case is detached, but still alive + EXPECT_EQ( size_t( 1 ), deltaEnsemble->allDerivedCases().size() ); + EXPECT_TRUE( guardedCase.notNull() ); + } + + EXPECT_FALSE( RimSummaryCaseUpdateBatch::isActive() ); + EXPECT_TRUE( guardedCase.isNull() ); +} + +//-------------------------------------------------------------------------------------------------- +/// +//-------------------------------------------------------------------------------------------------- +TEST_F( RimDeltaSummaryEnsembleTest, Batch_NestingFlushesOnce ) +{ + auto* ensemble1 = createEnsemble( "Ensemble 1", { 0, 1 } ); + auto* ensemble2 = createEnsemble( "Ensemble 2", { 0, 1 } ); + + auto* deltaEnsemble = createDeltaEnsemble( ensemble1, ensemble2 ); + deltaEnsemble->createDerivedEnsembleCases(); + ASSERT_EQ( size_t( 2 ), deltaEnsemble->allDerivedCases().size() ); + + caf::PdmPointer guardedCase = deltaEnsemble->allDerivedCases().front(); + + { + RimSummaryCaseUpdateBatch outerBatch; + + { + RimSummaryCaseUpdateBatch innerBatch; + + auto* sourceCase = ensemble1->allSummaryCases().front(); + mainCollection()->removeCase( sourceCase, false ); + delete sourceCase; + } + + // The inner scope contributes to the outer batch and must not flush + EXPECT_TRUE( guardedCase.notNull() ); + } + + EXPECT_TRUE( guardedCase.isNull() ); +} + //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- diff --git a/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp b/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp index d24e8598003..2d0089344bb 100644 --- a/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp +++ b/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp @@ -7,6 +7,7 @@ #include "RimDeltaSummaryEnsemble.h" #include "RimMockSummaryCase.h" #include "RimSummaryCaseMainCollection.h" +#include "RimSummaryCaseUpdateBatch.h" #include "RimSummaryEnsemble.h" #include "cafPdmPointer.h" @@ -30,10 +31,11 @@ RimSummaryCase* createMockCase( int realizationNumber ) } // namespace //-------------------------------------------------------------------------------------------------- -/// Removing a source case makes a delta ensemble recreate and delete derived cases. The derived cases -/// are part of the list of cases to remove, and must not be left as dangling pointers in that list. +/// Removing a source case makes a delta ensemble rebuild its derived cases. Those derived cases are +/// part of the list of cases to remove, and must stay alive for as long as the caller holds that +/// list. The caller states that span by opening a RimSummaryCaseUpdateBatch. //-------------------------------------------------------------------------------------------------- -TEST( RimSummaryCaseMainCollection, RemoveCasesReferredByDeltaEnsemble ) +TEST( RimSummaryCaseMainCollection, RemoveCases_NoDanglingInCallerVector ) { RimSummaryCaseMainCollection* mainCollection = RiaSummaryTools::summaryCaseMainCollection(); @@ -54,31 +56,31 @@ TEST( RimSummaryCaseMainCollection, RemoveCasesReferredByDeltaEnsemble ) // Guarded pointers are set to null when the object is deleted std::vector> guardedCases( cases.begin(), cases.end() ); - mainCollection->removeCases( cases ); + auto aliveCount = [&guardedCases]() + { + return static_cast( std::count_if( guardedCases.begin(), + guardedCases.end(), + []( const caf::PdmPointer& guardedCase ) + { return guardedCase.notNull(); } ) ); + }; - size_t aliveCount = 0; - for ( const auto& guardedCase : guardedCases ) { - if ( guardedCase.notNull() ) aliveCount++; - } + RimSummaryCaseUpdateBatch updateBatch; - // The two derived cases are deleted by the delta ensemble during removal - EXPECT_EQ( size_t( 4 ), aliveCount ); - EXPECT_EQ( aliveCount, cases.size() ); + mainCollection->removeCases( cases ); - // No deleted case is left behind in the list - for ( auto* summaryCase : cases ) - { - auto isAlive = [summaryCase]( const caf::PdmPointer& guardedCase ) - { return guardedCase.notNull() && guardedCase.p() == summaryCase; }; - EXPECT_TRUE( std::any_of( guardedCases.begin(), guardedCases.end(), isAlive ) ); - } + // The two derived cases are detached by the delta ensemble, but not destroyed while the batch is open + EXPECT_EQ( cases.size(), aliveCount() ); - for ( auto* summaryCase : cases ) - { - delete summaryCase; + for ( auto* summaryCase : cases ) + { + delete summaryCase; + } } + // Everything the caller handed over has been destroyed exactly once + EXPECT_EQ( size_t( 0 ), aliveCount() ); + mainCollection->removeEnsemble( deltaEnsemble ); mainCollection->removeEnsemble( ensemble1 ); mainCollection->removeEnsemble( ensemble2 ); From feafcfbe396df88b20fb7e880b148bebc57f7a78 Mon Sep 17 00:00:00 2001 From: Magne Sjaastad Date: Thu, 13 Aug 2026 15:29:13 +0200 Subject: [PATCH 5/6] #14517 Share the mock case factory between the summary tests RimDeltaSummaryEnsemble-Test.cpp and RimSummaryCaseMainCollection-Test.cpp each defined an identical createMockCase() in an anonymous namespace. A unity build concatenates the two translation units, which merges the two anonymous namespaces into one and makes the second definition a redefinition, breaking the build with C2084. Move the factory to RimMockSummaryCase.h, the header both tests already include for the mock case itself, and drop both local copies. --- .../UnitTests/RimDeltaSummaryEnsemble-Test.cpp | 14 -------------- .../UnitTests/RimMockSummaryCase.h | 16 ++++++++++++++++ .../RimSummaryCaseMainCollection-Test.cpp | 17 ----------------- 3 files changed, 16 insertions(+), 31 deletions(-) diff --git a/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp b/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp index 774e1454c87..75210e6db3d 100644 --- a/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp +++ b/ApplicationLibCode/UnitTests/RimDeltaSummaryEnsemble-Test.cpp @@ -2,8 +2,6 @@ #include "Summary/RiaSummaryTools.h" -#include "RigCaseRealizationParameters.h" - #include "RimDeltaSummaryEnsemble.h" #include "RimMockSummaryCase.h" #include "RimSummaryCaseMainCollection.h" @@ -15,22 +13,10 @@ #include "cafPdmPtrField.h" #include -#include #include namespace { -RimSummaryCase* createMockCase( int realizationNumber ) -{ - auto* summaryCase = new RimMockSummaryCase(); - - auto parameters = std::make_shared(); - parameters->setRealizationNumber( realizationNumber ); - summaryCase->setCaseRealizationParameters( parameters ); - - return summaryCase; -} - size_t countOf( const std::vector& ensembles, const RimDeltaSummaryEnsemble* ensemble ) { return static_cast( std::count( ensembles.begin(), ensembles.end(), ensemble ) ); diff --git a/ApplicationLibCode/UnitTests/RimMockSummaryCase.h b/ApplicationLibCode/UnitTests/RimMockSummaryCase.h index b835f87476d..6544395f7a1 100644 --- a/ApplicationLibCode/UnitTests/RimMockSummaryCase.h +++ b/ApplicationLibCode/UnitTests/RimMockSummaryCase.h @@ -1,9 +1,11 @@ #pragma once #include "RifSummaryReaderInterface.h" +#include "RigCaseRealizationParameters.h" #include "RimSummaryCase.h" #include +#include #include #include @@ -63,3 +65,17 @@ class RimMockSummaryCase : public RimSummaryCase, public RifSummaryReaderInterfa QString m_name = "MockCase"; std::map m_data; }; + +//-------------------------------------------------------------------------------------------------- +/// A mock case carrying the realization number a delta ensemble matches its source cases on. +//-------------------------------------------------------------------------------------------------- +inline RimSummaryCase* createMockCase( int realizationNumber ) +{ + auto* summaryCase = new RimMockSummaryCase(); + + auto parameters = std::make_shared(); + parameters->setRealizationNumber( realizationNumber ); + summaryCase->setCaseRealizationParameters( parameters ); + + return summaryCase; +} diff --git a/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp b/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp index 2d0089344bb..f186a17e9c0 100644 --- a/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp +++ b/ApplicationLibCode/UnitTests/RimSummaryCaseMainCollection-Test.cpp @@ -2,8 +2,6 @@ #include "Summary/RiaSummaryTools.h" -#include "RigCaseRealizationParameters.h" - #include "RimDeltaSummaryEnsemble.h" #include "RimMockSummaryCase.h" #include "RimSummaryCaseMainCollection.h" @@ -13,23 +11,8 @@ #include "cafPdmPointer.h" #include -#include #include -namespace -{ -RimSummaryCase* createMockCase( int realizationNumber ) -{ - auto* summaryCase = new RimMockSummaryCase(); - - auto parameters = std::make_shared(); - parameters->setRealizationNumber( realizationNumber ); - summaryCase->setCaseRealizationParameters( parameters ); - - return summaryCase; -} -} // namespace - //-------------------------------------------------------------------------------------------------- /// Removing a source case makes a delta ensemble rebuild its derived cases. Those derived cases are /// part of the list of cases to remove, and must stay alive for as long as the caller holds that From f03590fe75d49f89a89adc4d85171933b80ad92b Mon Sep 17 00:00:00 2001 From: Magne Sjaastad Date: Fri, 14 Aug 2026 07:39:22 +0200 Subject: [PATCH 6/6] #14423 Fail a unit test leaving data behind in the shared project The project is a global object shared by all tests, and the MSW export tests loaded a project without closing it. The two summary cases of that project stayed in the summary case main collection, and made RimSummaryCaseMainCollection.RemoveCases_NoDanglingInCallerVector fail on Windows only. Google Test runs the test suites in link order, and the MSW suite runs before the summary suite on Windows and after it on Linux. Add a test event listener reporting a failure for the test leaving cases, ensembles, well paths or views behind in the project. The project is closed as well, so the tests running after the offending one are unaffected. --- .../RicWellPathExportMswGeometryPath-Test.cpp | 15 ++++++ ApplicationLibCode/UnitTests/main.cpp | 53 +++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/ApplicationLibCode/UnitTests/RicWellPathExportMswGeometryPath-Test.cpp b/ApplicationLibCode/UnitTests/RicWellPathExportMswGeometryPath-Test.cpp index fccce1de839..b525e60a44e 100644 --- a/ApplicationLibCode/UnitTests/RicWellPathExportMswGeometryPath-Test.cpp +++ b/ApplicationLibCode/UnitTests/RicWellPathExportMswGeometryPath-Test.cpp @@ -451,6 +451,15 @@ struct MswExportInput RimWellPath* wellPath = nullptr; }; +//-------------------------------------------------------------------------------------------------- +/// The project is a global object shared by all tests, and must be closed before the test completes. +/// The tests below leave early on a failing assertion, so close from a destructor. +//-------------------------------------------------------------------------------------------------- +struct ProjectCloser +{ + ~ProjectCloser() { RiaApplication::instance()->closeProject(); } +}; + //-------------------------------------------------------------------------------------------------- /// Load the multiple_laterals project and look up the well path with the given name. /// Members are left as nullptr if the project, the case or the well path could not be found. @@ -489,6 +498,8 @@ MswExportInput loadMultipleLateralsProject( const QString& wellPathName ) //-------------------------------------------------------------------------------------------------- TEST( RicWellPathExportMswGeometryPath, MultipleLaterals_LateralsNumberedBeforeCompletionBranches ) { + ProjectCloser projectCloser; + auto input = loadMultipleLateralsProject( "Well-A Y1" ); ASSERT_TRUE( input.eclipseCase != nullptr ); ASSERT_TRUE( input.wellPath != nullptr ); @@ -520,6 +531,8 @@ TEST( RicWellPathExportMswGeometryPath, MultipleLaterals_LateralsNumberedBeforeC //-------------------------------------------------------------------------------------------------- TEST( RicWellPathExportMswGeometryPath, MultipleLaterals_CompletionBranchesListedAfterTheirLateral ) { + ProjectCloser projectCloser; + auto input = loadMultipleLateralsProject( "Well-A Y1" ); ASSERT_TRUE( input.eclipseCase != nullptr ); ASSERT_TRUE( input.wellPath != nullptr ); @@ -545,6 +558,8 @@ TEST( RicWellPathExportMswGeometryPath, MultipleLaterals_CompletionBranchesListe //-------------------------------------------------------------------------------------------------- TEST( RicWellPathExportMswGeometryPath, MultipleLaterals_CompsegsOrderedByBranchNumber ) { + ProjectCloser projectCloser; + auto input = loadMultipleLateralsProject( "Well-A Y1" ); ASSERT_TRUE( input.eclipseCase != nullptr ); ASSERT_TRUE( input.wellPath != nullptr ); diff --git a/ApplicationLibCode/UnitTests/main.cpp b/ApplicationLibCode/UnitTests/main.cpp index d244af9bfda..ae6359f9749 100644 --- a/ApplicationLibCode/UnitTests/main.cpp +++ b/ApplicationLibCode/UnitTests/main.cpp @@ -22,7 +22,55 @@ #include "RiaQuantityInfoTools.h" #include "RiaRegressionTestRunner.h" +#include "RimProject.h" + #include +#include + +//-------------------------------------------------------------------------------------------------- +/// The project is a global object shared by all tests. A test leaving data behind in the project +/// makes the outcome of the tests running after it depend on the test order, and the test order is +/// not the same on all platforms. +/// +/// Fail the test that leaves data behind, and close the project so the tests after it are unaffected. +//-------------------------------------------------------------------------------------------------- +class RiaProjectIsolationListener : public testing::EmptyTestEventListener +{ +private: + void OnTestEnd( const testing::TestInfo& ) override + { + RimProject* project = RimProject::current(); + if ( !project ) return; + + const QString leftovers = leftoverDescription( project ); + if ( leftovers.isEmpty() ) return; + + RiaApplication::instance()->closeProject(); + + ADD_FAILURE() << "The test left data behind in the shared project: " << leftovers.toStdString() + << ". Call RiaApplication::instance()->closeProject() before the test completes."; + } + + static QString leftoverDescription( RimProject* project ) + { + QStringList leftovers; + + auto appendCount = [&leftovers]( const QString& description, size_t count ) + { + if ( count > 0 ) leftovers.append( QString( "%1 %2" ).arg( count ).arg( description ) ); + }; + + appendCount( "grid case(s)", project->allGridCases().size() ); + appendCount( "summary case(s)", project->allSummaryCases().size() ); + appendCount( "summary ensemble(s)", project->summaryEnsembles().size() ); + appendCount( "well path(s)", project->allWellPaths().size() ); + appendCount( "view(s)", project->allViews().size() ); + + if ( !project->fileName().isEmpty() ) leftovers.append( "a project file name" ); + + return leftovers.join( ", " ); + } +}; //-------------------------------------------------------------------------------------------------- /// @@ -40,6 +88,11 @@ int main( int argc, char** argv ) setlocale( LC_NUMERIC, "C" ); testing::InitGoogleTest( &argc, argv ); + + // OnTestEnd is dispatched in reverse order of appending, so the listener appended last is the + // first to see the end of a test. The failure it reports is then part of the printed test result. + testing::UnitTest::GetInstance()->listeners().Append( new RiaProjectIsolationListener ); + int result = RUN_ALL_TESTS(); return result; }