From 495f958b9ac5c45196e743fcba300fcfd25b90a3 Mon Sep 17 00:00:00 2001 From: Sona Kurazyan Date: Thu, 26 Mar 2020 13:22:46 +0100 Subject: [PATCH] Store QFuture exceptions as std::exception_ptr Replaced the internal ExceptionHolder for storing QException* by std::exception_ptr. This will allow to report and store exceptions of types that are not derived from QException. Task-number: QTBUG-81588 Change-Id: I96be919d8289448b3e608310e51a16cebc586301 Reviewed-by: Vitaly Fanaskov Reviewed-by: Qt CI Bot --- src/concurrent/qtconcurrentthreadengine.cpp | 2 +- src/corelib/thread/qexception.cpp | 59 +++------- src/corelib/thread/qexception.h | 18 +-- src/corelib/thread/qfuture_impl.h | 7 +- src/corelib/thread/qfutureinterface.cpp | 9 ++ src/corelib/thread/qfutureinterface.h | 1 + .../corelib/thread/qfuture/tst_qfuture.cpp | 108 +++++++++++++++--- 7 files changed, 121 insertions(+), 83 deletions(-) diff --git a/src/concurrent/qtconcurrentthreadengine.cpp b/src/concurrent/qtconcurrentthreadengine.cpp index 7f91a2ba68a..3293c49507d 100644 --- a/src/concurrent/qtconcurrentthreadengine.cpp +++ b/src/concurrent/qtconcurrentthreadengine.cpp @@ -324,7 +324,7 @@ void ThreadEngineBase::handleException(const QException &exception) { if (futureInterface) futureInterface->reportException(exception); - else + else if (!exceptionStore.hasException()) exceptionStore.setException(exception); } #endif diff --git a/src/corelib/thread/qexception.cpp b/src/corelib/thread/qexception.cpp index f9c63085b75..5c7a05c9c2f 100644 --- a/src/corelib/thread/qexception.cpp +++ b/src/corelib/thread/qexception.cpp @@ -158,65 +158,38 @@ QUnhandledException *QUnhandledException::clone() const namespace QtPrivate { -class Base : public QSharedData -{ -public: - Base(QException *exception) - : exception(exception), hasThrown(false) { } - ~Base() { delete exception; } - - QException *exception; - bool hasThrown; -}; - -ExceptionHolder::ExceptionHolder(QException *exception) -: base(exception ? new Base(exception) : nullptr) {} - -ExceptionHolder::ExceptionHolder(const ExceptionHolder &other) -: base(other.base) -{} - -void ExceptionHolder::operator=(const ExceptionHolder &other) -{ - base = other.base; -} - -ExceptionHolder::~ExceptionHolder() -{} - -QException *ExceptionHolder::exception() const -{ - if (!base) - return nullptr; - return base->exception; -} - void ExceptionStore::setException(const QException &e) { - if (hasException() == false) - exceptionHolder = ExceptionHolder(e.clone()); + Q_ASSERT(!hasException()); + try { + e.raise(); + } catch (...) { + exceptionHolder = std::current_exception(); + } +} + +void ExceptionStore::setException(std::exception_ptr e) +{ + Q_ASSERT(!hasException()); + exceptionHolder = e; } bool ExceptionStore::hasException() const { - return (exceptionHolder.exception() != nullptr); + return !!exceptionHolder; } -ExceptionHolder ExceptionStore::exception() +std::exception_ptr ExceptionStore::exception() const { return exceptionHolder; } void ExceptionStore::throwPossibleException() { - if (hasException() ) { - exceptionHolder.base->hasThrown = true; - exceptionHolder.exception()->raise(); - } + if (hasException()) + std::rethrow_exception(exceptionHolder); } -bool ExceptionStore::hasThrown() const { return exceptionHolder.base->hasThrown; } - } // namespace QtPrivate #endif //Q_CLANG_QDOC diff --git a/src/corelib/thread/qexception.h b/src/corelib/thread/qexception.h index d33904c1f2d..b117d90caf9 100644 --- a/src/corelib/thread/qexception.h +++ b/src/corelib/thread/qexception.h @@ -84,27 +84,15 @@ public: namespace QtPrivate { -class Base; -class Q_CORE_EXPORT ExceptionHolder -{ -public: - ExceptionHolder(QException *exception = nullptr); - ExceptionHolder(const ExceptionHolder &other); - void operator=(const ExceptionHolder &other); // ### Qt6: copy-assign operator shouldn't return void. Remove this method and the copy-ctor, they are unneeded. - ~ExceptionHolder(); - QException *exception() const; - QExplicitlySharedDataPointer base; -}; - class Q_CORE_EXPORT ExceptionStore { public: void setException(const QException &e); + void setException(std::exception_ptr e); bool hasException() const; - ExceptionHolder exception(); + std::exception_ptr exception() const; void throwPossibleException(); - bool hasThrown() const; - ExceptionHolder exceptionHolder; + std::exception_ptr exceptionHolder; }; } // namespace QtPrivate diff --git a/src/corelib/thread/qfuture_impl.h b/src/corelib/thread/qfuture_impl.h index 20ee7e4f338..903b3787e3d 100644 --- a/src/corelib/thread/qfuture_impl.h +++ b/src/corelib/thread/qfuture_impl.h @@ -226,10 +226,8 @@ void Continuation::runFunction() } } #ifndef QT_NO_EXCEPTIONS - } catch (QException &e) { - promise.reportException(e); } catch (...) { - promise.reportException(QUnhandledException()); + promise.reportException(std::current_exception()); } #endif promise.reportFinished(); @@ -249,8 +247,7 @@ bool Continuation::execute() // interrupt the continuation chain, so don't report anything yet. if constexpr (!std::is_invocable_v, QFuture>) { promise.reportStarted(); - const QException *e = parentFuture.d.exceptionStore().exception().exception(); - promise.reportException(*e); + promise.reportException(parentFuture.d.exceptionStore().exception()); promise.reportFinished(); return false; } diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index 9d00bc32717..92b2df7c15f 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/src/corelib/thread/qfutureinterface.cpp @@ -282,6 +282,15 @@ void QFutureInterfaceBase::reportCanceled() #ifndef QT_NO_EXCEPTIONS void QFutureInterfaceBase::reportException(const QException &exception) +{ + try { + exception.raise(); + } catch (...) { + reportException(std::current_exception()); + } +} + +void QFutureInterfaceBase::reportException(std::exception_ptr exception) { QMutexLocker locker(&d->m_mutex); if (d->state.loadRelaxed() & (Canceled|Finished)) diff --git a/src/corelib/thread/qfutureinterface.h b/src/corelib/thread/qfutureinterface.h index 1da06beb7de..c8298d07423 100644 --- a/src/corelib/thread/qfutureinterface.h +++ b/src/corelib/thread/qfutureinterface.h @@ -90,6 +90,7 @@ public: void reportCanceled(); #ifndef QT_NO_EXCEPTIONS void reportException(const QException &e); + void reportException(std::exception_ptr e); #endif void reportResultsReady(int beginIndex, int endIndex); diff --git a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp index 7e314d63bae..4e207410550 100644 --- a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp +++ b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp @@ -1418,6 +1418,23 @@ QFuture createDerivedExceptionFuture() return f; } +struct TestException +{ +}; + +QFuture createCustomExceptionFuture() +{ + QFutureInterface i; + i.reportStarted(); + QFuture f = i.future(); + int r = 0; + i.reportResult(r); + auto exception = std::make_exception_ptr(TestException()); + i.reportException(exception); + i.reportFinished(); + return f; +} + void tst_QFuture::exceptions() { // test throwing from waitForFinished @@ -1502,6 +1519,18 @@ void tst_QFuture::exceptions() } QVERIFY(caught); } + + // Custom exceptions + { + QFuture f = createCustomExceptionFuture(); + bool caught = false; + try { + f.result(); + } catch (const TestException &) { + caught = true; + } + QVERIFY(caught); + } } class MyClass @@ -2040,46 +2069,87 @@ void tst_QFuture::thenOnExceptionFuture() } } +template +QFuture createExceptionContinuation(QtFuture::Launch policy = QtFuture::Launch::Sync) +{ + QFutureInterface promise; + + auto then = promise.future().then(policy, [] { + if constexpr (hasTestMsg) + throw Exception("TEST"); + else + throw Exception(); + }); + + promise.reportStarted(); + promise.reportFinished(); + + return then; +} + void tst_QFuture::thenThrows() { - // Continuation throws an exception + // Continuation throws a QException { - QFutureInterface promise; - - QFuture then = promise.future().then([]() { throw QException(); }); - - promise.reportStarted(); - promise.reportFinished(); + auto future = createExceptionContinuation(); bool caught = false; try { - then.waitForFinished(); - } catch (QException &) { + future.waitForFinished(); + } catch (const QException &) { caught = true; } QVERIFY(caught); } + // Continuation throws an exception derived from QException + { + auto future = createExceptionContinuation(); + + bool caught = false; + try { + future.waitForFinished(); + } catch (const QException &) { + caught = true; + } catch (const std::exception &) { + QFAIL("The exception should be caught by the above catch block."); + } + + QVERIFY(caught); + } + + // Continuation throws std::exception + { + auto future = createExceptionContinuation(); + + bool caught = false; + try { + future.waitForFinished(); + } catch (const QException &) { + QFAIL("The exception should be caught by the below catch block."); + } catch (const std::exception &e) { + QCOMPARE(e.what(), "TEST"); + caught = true; + } + + QVERIFY(caught); + } + // Same with QtFuture::Launch::Async { - QFutureInterface promise; - - QFuture then = - promise.future().then(QtFuture::Launch::Async, []() { throw QException(); }); - - promise.reportStarted(); - promise.reportFinished(); + auto future = createExceptionContinuation(QtFuture::Launch::Async); bool caught = false; try { - then.waitForFinished(); - } catch (QException &) { + future.waitForFinished(); + } catch (const QException &) { caught = true; } QVERIFY(caught); } } -#endif + +#endif // QT_NO_EXCEPTIONS void tst_QFuture::testSingleResult(const UniquePtr &p) {