From 695a314229e69c4485bba997331369371bf4e8ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Caldas?= Date: Tue, 19 Sep 2023 02:16:45 -0300 Subject: [PATCH] TechDraw: creates closure for concurrent thread context. We use a lambda function with a copy of variables that might be destructed in the original calling thread, possibly producing dangling references. See: https://forum.freecad.org/viewtopic.php?t=81260 --- src/Mod/TechDraw/App/DrawComplexSection.cpp | 11 ++++---- src/Mod/TechDraw/App/DrawViewDetail.cpp | 12 +++++---- src/Mod/TechDraw/App/DrawViewPart.cpp | 28 +++++++-------------- src/Mod/TechDraw/App/DrawViewSection.cpp | 11 ++++---- 4 files changed, 28 insertions(+), 34 deletions(-) diff --git a/src/Mod/TechDraw/App/DrawComplexSection.cpp b/src/Mod/TechDraw/App/DrawComplexSection.cpp index 2e20052a4e..ebcfb76901 100644 --- a/src/Mod/TechDraw/App/DrawComplexSection.cpp +++ b/src/Mod/TechDraw/App/DrawComplexSection.cpp @@ -281,11 +281,12 @@ void DrawComplexSection::makeSectionCut(const TopoDS_Shape& baseShape) connectAlignWatcher = QObject::connect(&m_alignWatcher, &QFutureWatcherBase::finished, &m_alignWatcher, [this] { this->onSectionCutFinished(); }); -#if QT_VERSION < QT_VERSION_CHECK(6,0,0) - m_alignFuture = QtConcurrent::run(this, &DrawComplexSection::makeAlignedPieces, baseShape); -#else - m_alignFuture = QtConcurrent::run(&DrawComplexSection::makeAlignedPieces, this, baseShape); -#endif + + // We create a lambda closure to hold a copy of baseShape. + // This is important because this variable might be local to the calling + // function and might get destructed before the parallel processing finishes. + auto lambda = [this, baseShape]{this->makeAlignedPieces(baseShape);}; + m_alignFuture = QtConcurrent::run(std::move(lambda)); m_alignWatcher.setFuture(m_alignFuture); waitingForAlign(true); } diff --git a/src/Mod/TechDraw/App/DrawViewDetail.cpp b/src/Mod/TechDraw/App/DrawViewDetail.cpp index 5da9a48fde..5f5cab16a3 100644 --- a/src/Mod/TechDraw/App/DrawViewDetail.cpp +++ b/src/Mod/TechDraw/App/DrawViewDetail.cpp @@ -187,11 +187,13 @@ void DrawViewDetail::detailExec(TopoDS_Shape& shape, DrawViewPart* dvp, DrawView connectDetailWatcher = QObject::connect(&m_detailWatcher, &QFutureWatcherBase::finished, &m_detailWatcher, [this] { this->onMakeDetailFinished(); }); -#if QT_VERSION < QT_VERSION_CHECK(6,0,0) - m_detailFuture = QtConcurrent::run(this, &DrawViewDetail::makeDetailShape, shape, dvp, dvs); -#else - m_detailFuture = QtConcurrent::run(&DrawViewDetail::makeDetailShape, this, shape, dvp, dvs); -#endif + + // We create a lambda closure to hold a copy of shape. + // This is important because this variable might be local to the calling + // function and might get destructed before the parallel processing finishes. + // TODO: What about dvp and dvs? Do they live past makeDetailShape? + auto lambda = [this, shape, dvp, dvs]{this->makeDetailShape(shape, dvp, dvs);}; + m_detailFuture = QtConcurrent::run(std::move(lambda)); m_detailWatcher.setFuture(m_detailFuture); waitingForDetail(true); } diff --git a/src/Mod/TechDraw/App/DrawViewPart.cpp b/src/Mod/TechDraw/App/DrawViewPart.cpp index 82a085a091..e6f39aef75 100644 --- a/src/Mod/TechDraw/App/DrawViewPart.cpp +++ b/src/Mod/TechDraw/App/DrawViewPart.cpp @@ -369,26 +369,18 @@ TechDraw::GeometryObjectPtr DrawViewPart::buildGeometryObject(TopoDS_Shape& shap go->projectShapeWithPolygonAlgo(shape, viewAxis); } else { - // TODO: we should give the thread its own copy of the shape because the passed one will be - // destroyed when the call stack we followed to get here unwinds and the thread could still be - // running. - // Should we pass a smart pointer instead of const& ?? - // bool copyGeometry = true; - // bool copyMesh = false; - // BRepBuilderAPI_Copy copier(shape, copyGeometry, copyMesh); - // copier.Shape(); - //projectShape (the HLR process) runs in a separate thread since it can take a long time //note that &m_hlrWatcher in the third parameter is not strictly required, but using the //4 parameter signature instead of the 3 parameter signature prevents clazy warning: //https://github.com/KDE/clazy/blob/1.11/docs/checks/README-connect-3arg-lambda.md connectHlrWatcher = QObject::connect(&m_hlrWatcher, &QFutureWatcherBase::finished, &m_hlrWatcher, [this] { this->onHlrFinished(); }); -#if QT_VERSION < QT_VERSION_CHECK(6,0,0) - m_hlrFuture = QtConcurrent::run(go.get(), &GeometryObject::projectShape, shape, viewAxis); -#else - m_hlrFuture = QtConcurrent::run(&GeometryObject::projectShape, go.get(), shape, viewAxis); -#endif + + // We create a lambda closure to hold a copy of go, shape and viewAxis. + // This is important because those variables might be local to the calling + // function and might get destructed before the parallel processing finishes. + auto lambda = [go, shape, viewAxis]{go->projectShape(shape, viewAxis);}; + m_hlrFuture = QtConcurrent::run(std::move(lambda)); m_hlrWatcher.setFuture(m_hlrFuture); waitingForHlr(true); } @@ -428,11 +420,9 @@ void DrawViewPart::onHlrFinished(void) connectFaceWatcher = QObject::connect(&m_faceWatcher, &QFutureWatcherBase::finished, &m_faceWatcher, [this] { this->onFacesFinished(); }); -#if QT_VERSION < QT_VERSION_CHECK(6,0,0) - m_faceFuture = QtConcurrent::run(this, &DrawViewPart::extractFaces); -#else - m_faceFuture = QtConcurrent::run(&DrawViewPart::extractFaces, this); -#endif + + auto lambda = [this]{this->extractFaces();}; + m_faceFuture = QtConcurrent::run(std::move(lambda)); m_faceWatcher.setFuture(m_faceFuture); waitingForFaces(true); } diff --git a/src/Mod/TechDraw/App/DrawViewSection.cpp b/src/Mod/TechDraw/App/DrawViewSection.cpp index f29486b417..cb72d0d190 100644 --- a/src/Mod/TechDraw/App/DrawViewSection.cpp +++ b/src/Mod/TechDraw/App/DrawViewSection.cpp @@ -462,11 +462,12 @@ void DrawViewSection::sectionExec(TopoDS_Shape& baseShape) QObject::connect(&m_cutWatcher, &QFutureWatcherBase::finished, &m_cutWatcher, [this] { this->onSectionCutFinished(); }); -#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0) - m_cutFuture = QtConcurrent::run(this, &DrawViewSection::makeSectionCut, baseShape); -#else - m_cutFuture = QtConcurrent::run(&DrawViewSection::makeSectionCut, this, baseShape); -#endif + + // We create a lambda closure to hold a copy of baseShape. + // This is important because this variable might be local to the calling + // function and might get destructed before the parallel processing finishes. + auto lambda = [this, baseShape]{this->makeSectionCut(baseShape);}; + m_cutFuture = QtConcurrent::run(std::move(lambda)); m_cutWatcher.setFuture(m_cutFuture); waitingForCut(true); }