From 3be72253a6c8f469626d931b623f548cfe1e81a7 Mon Sep 17 00:00:00 2001 From: Sona Kurazyan Date: Mon, 29 Nov 2021 17:36:57 +0100 Subject: [PATCH] Fix QFuture continuations/handlers to work with move-only callables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit std::function, which is used to store the type-erased continuation lambdas, requires the passed callable to be copy-constructible. This makes impossible to use move-only callables with continuations/handlers. In particular, it makes impossible passing lambdas that are capturing move-only objects. The workaround is to store the continuation lambda inside a wrapper for the callable, which stores the move-only lambda in a QSharedPtr and can be stored in std::function, since it's copyable. Pick-to: 6.2 Fixes: QTBUG-98493 Change-Id: I8b7a22fcf68dc132b3c533216a7a1665e9f9fb0a Reviewed-by: Fabian Kosmale Reviewed-by: Mårten Nordheim --- src/corelib/thread/qfuture_impl.h | 51 +++++++++++--- .../corelib/thread/qfuture/CMakeLists.txt | 5 ++ .../corelib/thread/qfuture/tst_qfuture.cpp | 66 +++++++++++++++++++ 3 files changed, 114 insertions(+), 8 deletions(-) diff --git a/src/corelib/thread/qfuture_impl.h b/src/corelib/thread/qfuture_impl.h index e5a15b57ff..136892ddaf 100644 --- a/src/corelib/thread/qfuture_impl.h +++ b/src/corelib/thread/qfuture_impl.h @@ -498,6 +498,17 @@ bool Continuation::execute() return true; } +// Workaround for keeping move-only lambdas inside std::function +template +struct ContinuationWrapper +{ + ContinuationWrapper(Function &&f) : function(QSharedPointer::create(std::move(f))) { } + void operator()(const QFutureInterfaceBase &parentData) { (*function)(parentData); } + +private: + QSharedPointer function; +}; + template template void Continuation::create(F &&func, @@ -543,8 +554,10 @@ void Continuation::create(F &&func, continuationJob = nullptr; } }; - - f->d.setContinuation(std::move(continuation), p.d); + if constexpr (!std::is_copy_constructible_v) + f->d.setContinuation(ContinuationWrapper(std::move(continuation)), p.d); + else + f->d.setContinuation(std::move(continuation), p.d); } template @@ -573,7 +586,10 @@ void Continuation::create(F &&func, } }; - f->d.setContinuation(std::move(continuation), p.d); + if constexpr (!std::is_copy_constructible_v) + f->d.setContinuation(ContinuationWrapper(std::move(continuation)), p.d); + else + f->d.setContinuation(std::move(continuation), p.d); } template @@ -596,7 +612,10 @@ void Continuation::create(F &&func, }); }; - f->d.setContinuation(std::move(continuation), p.d); + if constexpr (!std::is_copy_constructible_v) + f->d.setContinuation(ContinuationWrapper(std::move(continuation)), p.d); + else + f->d.setContinuation(std::move(continuation), p.d); } template @@ -675,7 +694,10 @@ void FailureHandler::create(F &&function, QFutured.setContinuation(std::move(failureContinuation)); + if constexpr (!std::is_copy_constructible_v) + future->d.setContinuation(ContinuationWrapper(std::move(failureContinuation))); + else + future->d.setContinuation(std::move(failureContinuation)); } template @@ -699,7 +721,10 @@ void FailureHandler::create(F &&function, QFutured.setContinuation(std::move(failureContinuation)); + if constexpr (!std::is_copy_constructible_v) + future->d.setContinuation(ContinuationWrapper(std::move(failureContinuation))); + else + future->d.setContinuation(std::move(failureContinuation)); } template @@ -782,7 +807,12 @@ public: auto parentFuture = QFutureInterface(parentData).future(); run(std::forward(handler), parentFuture, promise); }; - future->d.setContinuation(std::move(canceledContinuation)); + + if constexpr (!std::is_copy_constructible_v) + future->d.setContinuation(ContinuationWrapper(std::move(canceledContinuation))); + else + future->d.setContinuation(std::move(canceledContinuation)); + return promise.future(); } @@ -802,7 +832,12 @@ public: run(std::forward(handler), parentFuture, promise); }); }; - future->d.setContinuation(std::move(canceledContinuation)); + + if constexpr (!std::is_copy_constructible_v) + future->d.setContinuation(ContinuationWrapper(std::move(canceledContinuation))); + else + future->d.setContinuation(std::move(canceledContinuation)); + return promise.future(); } diff --git a/tests/auto/corelib/thread/qfuture/CMakeLists.txt b/tests/auto/corelib/thread/qfuture/CMakeLists.txt index dcf7846fd2..df5826032d 100644 --- a/tests/auto/corelib/thread/qfuture/CMakeLists.txt +++ b/tests/auto/corelib/thread/qfuture/CMakeLists.txt @@ -12,3 +12,8 @@ qt_internal_add_test(tst_qfuture PUBLIC_LIBRARIES Qt::CorePrivate ) + +qt_internal_extend_target(tst_qmetatype CONDITION MSVC + COMPILE_OPTIONS + /bigobj +) diff --git a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp index 385fbe7fca..f97caa0633 100644 --- a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp +++ b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp @@ -145,6 +145,7 @@ private slots: void onCanceled(); void cancelContinuations(); void continuationsWithContext(); + void continuationsWithMoveOnlyLambda(); #if 0 // TODO: enable when QFuture::takeResults() is enabled void takeResults(); @@ -3111,6 +3112,71 @@ void tst_QFuture::continuationsWithContext() thread.wait(); } +void tst_QFuture::continuationsWithMoveOnlyLambda() +{ + // .then() + { + std::unique_ptr uniquePtr(new int(42)); + auto future = QtFuture::makeReadyFuture().then([p = std::move(uniquePtr)] { return *p; }); + QCOMPARE(future.result(), 42); + } + // .then() with thread pool + { + QThreadPool pool; + + std::unique_ptr uniquePtr(new int(42)); + auto future = + QtFuture::makeReadyFuture().then(&pool, [p = std::move(uniquePtr)] { return *p; }); + QCOMPARE(future.result(), 42); + } + // .then() with context + { + QObject object; + + std::unique_ptr uniquePtr(new int(42)); + auto future = QtFuture::makeReadyFuture().then(&object, + [p = std::move(uniquePtr)] { return *p; }); + QCOMPARE(future.result(), 42); + } + + // .onCanceled() + { + std::unique_ptr uniquePtr(new int(42)); + auto future = + createCanceledFuture().onCanceled([p = std::move(uniquePtr)] { return *p; }); + QCOMPARE(future.result(), 42); + } + + // .onCanceled() with context + { + QObject object; + + std::unique_ptr uniquePtr(new int(42)); + auto future = createCanceledFuture().onCanceled( + &object, [p = std::move(uniquePtr)] { return *p; }); + QCOMPARE(future.result(), 42); + } + +#ifndef QT_NO_EXCEPTIONS + // .onFailed() + { + std::unique_ptr uniquePtr(new int(42)); + auto future = QtFuture::makeExceptionalFuture(QException()) + .onFailed([p = std::move(uniquePtr)] { return *p; }); + QCOMPARE(future.result(), 42); + } + // .onFailed() with context + { + QObject object; + + std::unique_ptr uniquePtr(new int(42)); + auto future = QtFuture::makeExceptionalFuture(QException()) + .onFailed(&object, [p = std::move(uniquePtr)] { return *p; }); + QCOMPARE(future.result(), 42); + } +#endif // QT_NO_EXCEPTIONS +} + void tst_QFuture::testSingleResult(const UniquePtr &p) { QVERIFY(p.get() != nullptr);