Remove AsyncPdmObjectVectorDeleter and delete PDM objects synchronously - #14492
Merged
Conversation
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.
The include came transitively from cafAsyncObjectDeleter.inl through cafPdmChildArrayField.inl. Removing the async deleter broke the build on Linux, where libstdc++ and libc++ do not pull in <thread> the way the MSVC STL does.
kriben
approved these changes
Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes
caf::AsyncPdmObjectVectorDeleterandPdmChildArrayField::deleteChildrenAsync(), and deletes PDM objects synchronously instead. Closes #14491.Problem
Deleting a PDM object is not a local operation.
PdmObjectHandle::prepareForDelete()mutates state owned by other objects:m_pointersReferencingMeis an unsynchronisedstd::setthatPdmPointerImpl::addReference()andremoveReference()touch on everyPdmPointerconstruction, copy, assignment and destruction. Destroying an object on a worker thread therefore races with the main thread on the shared object graph.Async deletion also nested:
~RimSummaryCaseMainCollectioncalledm_ensembles.deleteChildrenAsync(), the spawned thread ran~RimSummaryEnsemble, which calledm_cases.deleteChildrenAsync()again. Crash reports from release 2026.06.1:Why removal rather than hardening
The mechanism does not pay for itself. Measured teardown of already loaded summary readers, Windows, MSVC, system allocator:
free()is serialised inside the allocator, so at ensemble scale the speedup disappears, and for large payloads parallel release is slower than sequential. Teardown is cheap in absolute terms either way, and the PDM bookkeeping the race is about accounts for 0.4 to 2.8 percent of it.Caveat on the numbers: the readers were loaded on one thread, while
loadFileSummaryCaseData()loads them with OpenMP. Under an allocator with per-thread arenas, glibc for instance, that changes which arena the memory is freed to and could change the parallel column. The measurement is from Windows, the crash reports are from Linux builds.Changes
cafAsyncObjectDeleter.hand.inl, andPdmChildArrayField::deleteChildrenAsync().caf::PdmObjectHandleTools::deleteObjects()for deleting a vector of objects that is no longer owned by a child array field. Used byRicCloseSummaryCaseFeature.RimSummaryCaseMainCollection::~,RimSummaryEnsemble::~,RimSummaryEnsemble::replaceCases()andRimSummaryFileSetEnsemble::createSummaryCasesFromEnsembleFileSet()usedclearWithoutDelete()plus a manual delete loop to work around the race. They now calldeleteChildren()directly.Note on issue 12262
The observer disconnection does not depend on
clearWithoutDelete().~Signal()callsremoveObservedSignal()on every observer, so a deleted child detaches itself:Verified by asserting the observed signal count with and without
clearWithoutDelete()indeleteChildren(), it reaches zero either way. #12262 was a race in the async path, where~Signal()mutated the observer list from a worker thread concurrently with the main thread. With deletion synchronous the ordering is deterministic, sodeleteChildren()keeps its original body.The unit test is renamed to
DeletedChildrenDisconnectFromObserverand now asserts the observed signal count directly, 1000 before deletion and 0 after, instead of relying on a crash that only reproduced in a Debug build.Verification
ResInsightandResInsight-testsbuild and link, all 90cafPdmCore_UnitTestspass.caf::AsyncWorkerManagerhas no callers left after this change. It is a separate general purpose utility, so it is left in place.