This is an automated email from the ASF dual-hosted git repository. pnoltes pushed a commit to branch feature/robust_promise_fail_and_resolve in repository https://gitbox.apache.org/repos/asf/celix.git
commit 43296771b1fc77fbf83fc4f1ffa4380331542900 Author: Pepijn Noltes <[email protected]> AuthorDate: Tue Oct 5 17:52:26 2021 +0200 Updates Promise `resolve` and `fail` so make them robust. Also add a `tryResolve` and `tryFail` alternative to be able to resolve a promise and check if it was already resolved. --- libs/promises/README.md | 11 +- libs/promises/api/celix/Deferred.h | 181 +++++++++++++++++---- libs/promises/api/celix/Promise.h | 10 +- libs/promises/api/celix/impl/SharedPromiseState.h | 184 ++++++++-------------- libs/promises/gtest/src/PromisesTestSuite.cc | 123 ++++++++++++++- libs/promises/gtest/src/VoidPromisesTestSuite.cc | 102 ++++++++++++ 6 files changed, 454 insertions(+), 157 deletions(-) diff --git a/libs/promises/README.md b/libs/promises/README.md index 3197cec..dd7f1bd 100644 --- a/libs/promises/README.md +++ b/libs/promises/README.md @@ -78,11 +78,16 @@ target_link_libraries(PromiseExamples PRIVATE Celix::Promises) 3. The default constructor for celix::Deferred has been removed. A celix:Deferred can only be created through a PromiseFactory. This is done because the promise concept is heavily bound with the execution abstraction and thus a execution model. Creating a Deferred without a explicit executor is not desirable. 4. The PromiseFactory also has a deferredTask method. This is a convenient method create a Deferred, execute a task async to resolve the Deferred and return a Promise of the created Deferred in one call. 5. The celix::IExecutor abstraction has a priority argument (and as result also the calls in PromiseFactory, etc). -6. The IExecutor has a added wait() method. This can be used to ensure a executor is done executing the tasks backlog. - - +6. The IExecutor has a added wait() method. This can be used to ensure an executor is done executing the tasks backlog. +7. The `celix::Deferred<T>::fail` and `celix::Deferred<T>::resolve` are make robust for resolving a + deferred if the associated promise is already resolved. This is different from the OSGi spec, + because it always a race condition to check if a promise is already resolved (`isDone()`) + and then resolve the deferred. The methods `celix::Deferred<T>::tryFail` and + `celix::Deferred<T>::tryResolve` exist to resolve a deferred and check if it was + already resolved atomically. ## Open Issues & TODOs + - Documentation not complete - PromiseFactory is not complete yet - The static helper class Promises is not implemented yet (e.g. all/any) diff --git a/libs/promises/api/celix/Deferred.h b/libs/promises/api/celix/Deferred.h index bf8f1ff..eb07091 100644 --- a/libs/promises/api/celix/Deferred.h +++ b/libs/promises/api/celix/Deferred.h @@ -53,7 +53,8 @@ namespace celix { explicit Deferred(std::shared_ptr<celix::impl::SharedPromiseState<T>> state); /** - * Fail the Promise associated with this Deferred. + * @brief Fail the Promise associated with this Deferred. + * * <p/> * After the associated Promise is resolved with the specified failure, all registered callbacks are called and any * chained Promises are resolved. @@ -61,13 +62,23 @@ namespace celix { * Resolving the associated Promise happens-before any registered callback is called. That is, in a registered * callback, Promise.isDone() must return true and Promise.getValue() and Promise.getFailure() must not block. * + * If the associated promise is already resolved, the call will be ignored. + * * @param failure The failure in the form of an exception pointer. - * @throws PromiseInvocationException If the associated Promise was already resolved. */ void fail(std::exception_ptr failure); /** - * Fail the Promise associated with this Deferred. + * @brief Try to fail the Promise associated with this Deferred. + * + * Same as `fail`, but will return `true` if the associated promise was successfully failed and `false` if + * the associated promise was already resolved. + */ + bool tryFail(std::exception_ptr failure); + + /** + * @brief Fail the Promise associated with this Deferred. + * * <p/> * After the associated Promise is resolved with the specified failure, all registered callbacks are called and any * chained Promises are resolved. @@ -75,24 +86,38 @@ namespace celix { * Resolving the associated Promise happens-before any registered callback is called. That is, in a registered * callback, Promise.isDone() must return true and Promise.getValue() and Promise.getFailure() must not block. * + * If the associated promise is already resolved, the call will be ignored. + * * @param failure The failure in the form of an const std::exception reference. - * @throws PromiseInvocationException If the associated Promise was already resolved. */ - template<typename E, typename std::enable_if_t< std::is_base_of<std::exception, E>::value, bool> = true > + template<typename E, typename std::enable_if_t< std::is_base_of_v<std::exception, E>, bool> = true > void fail(const E& failure); + /** - * Returns the Promise associated with this Deferred. + * @brief Try to fail the Promise associated with this Deferred. + * + * Same as `fail`, but will return `true` if the associated promise was successfully failed and `false` if + * the associated promise was already resolved. + */ + template<typename E, typename std::enable_if_t< std::is_base_of_v<std::exception, E>, bool> = true > + bool tryFail(const E& failure); + + /** + * @brief Returns the Promise associated with this Deferred. + * * <p> * All Promise objects created by the associated Promise will use the * executors of the associated Promise. + * </p> * * @return The Promise associated with this Deferred. */ [[nodiscard]] Promise<T> getPromise(); /** - * Successfully resolve the Promise associated with this Deferred. + * @brief Resolve the Promise associated with this Deferred. + * * <p/> * After the associated Promise is resolved with the specified value, all registered callbacks are called and any * chained Promises are resolved. @@ -100,14 +125,47 @@ namespace celix { * Resolving the associated Promise happens-before any registered callback is called. That is, in a registered * callback, Promise.isDone() must return true and Promise.getValue() and Promise.getFailure() must not block. * + * If the associated promise is already resolved, the call will be ignored. + * * @param value The value of the resolved Promise. - * @throws PromiseInvocationException If the associated Promise was already resolved. */ void resolve(T&& value); + + /** + * @brief Resolve the Promise associated with this Deferred. + * + * <p/> + * After the associated Promise is resolved with the specified value, all registered callbacks are called and any + * chained Promises are resolved. + * <p/> + * Resolving the associated Promise happens-before any registered callback is called. That is, in a registered + * callback, Promise.isDone() must return true and Promise.getValue() and Promise.getFailure() must not block. + * + * If the associated promise is already resolved, the call will be ignored. + * + * @param value The value of the resolved Promise. + */ void resolve(const T& value); /** - * Resolve the Promise associated with this Deferred with the specified Promise. + * @brief Try to resolve the Promise associated with this Deferred. + * + * Same as `resolve`, but will return `true` if the associated promise was successfully resolved and `false` if + * the associated promise was already resolved. + */ + bool tryResolve(T&& value); + + /** + * @brief Try to resolve the Promise associated with this Deferred. + * + * Same as `resolve`, but will return `true` if the associated promise was successfully resolved and `false` if + * the associated promise was already resolved. + */ + bool tryResolve(const T& value); + + /** + * @brief Resolve the Promise associated with this Deferred with the specified Promise. + * * <p/> * If the specified Promise is successfully resolved, the associated Promise is resolved with the value of the * specified Promise. If the specified Promise is resolved with a failure, the associated Promise is resolved with @@ -140,10 +198,9 @@ namespace celix { explicit Deferred(std::shared_ptr<celix::impl::SharedPromiseState<void>> state); - //TODO deferred ctor with factory - /** - * Fail the Promise associated with this Deferred. + * @brief Fail the Promise associated with this Deferred. + * * <p/> * After the associated Promise is resolved with the specified failure, all registered callbacks are called and any * chained Promises are resolved. @@ -151,13 +208,23 @@ namespace celix { * Resolving the associated Promise happens-before any registered callback is called. That is, in a registered * callback, Promise.isDone() must return true and Promise.getValue() and Promise.getFailure() must not block. * + * If the associated promise is already resolved, the call will be ignored. + * * @param failure The failure in the form of an exception pointer. - * @throws PromiseInvocationException If the associated Promise was already resolved. */ void fail(std::exception_ptr failure); /** - * Fail the Promise associated with this Deferred. + * @brief Try to fail the Promise associated with this Deferred. + * + * Same as `fail`, but will return `true` if the associated promise was successfully failed and `false` if + * the associated promise was already resolved. + */ + bool tryFail(std::exception_ptr failure); + + /** + * @brief Fail the Promise associated with this Deferred. + * * <p/> * After the associated Promise is resolved with the specified failure, all registered callbacks are called and any * chained Promises are resolved. @@ -165,24 +232,37 @@ namespace celix { * Resolving the associated Promise happens-before any registered callback is called. That is, in a registered * callback, Promise.isDone() must return true and Promise.getValue() and Promise.getFailure() must not block. * + * If the associated promise is already resolved, the call will be ignored. + * * @param failure The failure in the form of an const std::exception reference. - * @throws PromiseInvocationException If the associated Promise was already resolved. */ - template<typename E, typename std::enable_if_t< std::is_base_of<std::exception, E>::value, bool> = true > + template<typename E, typename std::enable_if_t< std::is_base_of_v<std::exception, E>, bool> = true > void fail(const E& failure); /** - * Returns the Promise associated with this Deferred. + * @brief Try to fail the Promise associated with this Deferred. + * + * Same as `fail`, but will return `true` if the associated promise was successfully failed and `false` if + * the associated promise was already resolved. + */ + template<typename E, typename std::enable_if_t< std::is_base_of_v<std::exception, E>, bool> = true > + bool tryFail(const E& failure); + + /** + * @brief Returns the Promise associated with this Deferred. + * * <p> * All Promise objects created by the associated Promise will use the * executors of the associated Promise. + * </p> * * @return The Promise associated with this Deferred. */ [[nodiscard]] Promise<void> getPromise(); /** - * Successfully resolve the Promise associated with this Deferred. + * @brief Resolve the Promise associated with this Deferred. + * * <p/> * After the associated Promise is resolved with the specified value, all registered callbacks are called and any * chained Promises are resolved. @@ -190,11 +270,20 @@ namespace celix { * Resolving the associated Promise happens-before any registered callback is called. That is, in a registered * callback, Promise.isDone() must return true and Promise.getValue() and Promise.getFailure() must not block. * + * If the associated promise is already resolved, the call will be ignored. + * * @param value The value of the resolved Promise. - * @throws PromiseInvocationException If the associated Promise was already resolved. */ void resolve(); + /** + * @brief Try to resolve the Promise associated with this Deferred. + * + * Same as `resolve`, but will return `true` if the associated promise was successfully resolved and `false` if + * the associated promise was already resolved. + */ + bool tryResolve(); + template<typename U> celix::Promise<void> resolveWith(celix::Promise<U> with); private: @@ -215,22 +304,42 @@ inline celix::Deferred<void>::Deferred(std::shared_ptr<celix::impl::SharedPromis template<typename T> void celix::Deferred<T>::fail(std::exception_ptr failure) { - state->fail(std::move(failure)); + state->tryFail(std::move(failure)); +} + +template<typename T> +bool celix::Deferred<T>::tryFail(std::exception_ptr failure) { + return state->tryFail(std::move(failure)); } inline void celix::Deferred<void>::fail(std::exception_ptr failure) { - state->fail(std::move(failure)); + state->tryFail(std::move(failure)); +} + +inline bool celix::Deferred<void>::tryFail(std::exception_ptr failure) { + return state->tryFail(std::move(failure)); } template<typename T> -template<typename E, typename std::enable_if_t< std::is_base_of<std::exception, E>::value, bool>> +template<typename E, typename std::enable_if_t< std::is_base_of_v<std::exception, E>, bool>> void celix::Deferred<T>::fail(const E& failure) { - state->template fail<E>(failure); + state->template tryFail<E>(failure); +} + +template<typename T> +template<typename E, typename std::enable_if_t< std::is_base_of_v<std::exception, E>, bool>> +bool celix::Deferred<T>::tryFail(const E& failure) { + return state->template tryFail<E>(failure); } -template<typename E, typename std::enable_if_t< std::is_base_of<std::exception, E>::value, bool>> -inline void celix::Deferred<void>::fail(const E& failure) { - state->template fail<E>(failure); +template<typename E, typename std::enable_if_t< std::is_base_of_v<std::exception, E>, bool>> +void celix::Deferred<void>::fail(const E& failure) { + state->tryFail<E>(failure); +} + +template<typename E, typename std::enable_if_t< std::is_base_of_v<std::exception, E>, bool>> +bool celix::Deferred<void>::tryFail(const E& failure) { + return state->tryFail<E>(failure); } template<typename T> @@ -286,14 +395,28 @@ inline celix::Promise<void> celix::Deferred<void>::resolveWith(celix::Promise<U> template<typename T> void celix::Deferred<T>::resolve(T&& value) { - state->resolve(std::forward<T>(value)); + state->tryResolve(std::forward<T>(value)); } template<typename T> void celix::Deferred<T>::resolve(const T& value) { - state->resolve(value); + state->tryResolve(value); } inline void celix::Deferred<void>::resolve() { - state->resolve(); + state->tryResolve(); +} + +template<typename T> +bool celix::Deferred<T>::tryResolve(T&& value) { + return state->tryResolve(std::forward<T>(value)); } + +template<typename T> +bool celix::Deferred<T>::tryResolve(const T& value) { + return state->tryResolve(value); +} + +inline bool celix::Deferred<void>::tryResolve() { + return state->tryResolve(); +} \ No newline at end of file diff --git a/libs/promises/api/celix/Promise.h b/libs/promises/api/celix/Promise.h index 1ce6b32..3d1312d 100644 --- a/libs/promises/api/celix/Promise.h +++ b/libs/promises/api/celix/Promise.h @@ -745,14 +745,13 @@ inline celix::Promise<U> celix::Promise<T>::then(std::function<celix::Promise<U> auto tmpPromise = success(celix::Promise<T>{s}); p->resolveWith(*tmpPromise.state); } catch (...) { - //failure(); TODO not sure if this needs to be called - p->fail(std::current_exception()); + p->tryFail(std::current_exception()); } } else { if (failure) { failure(celix::Promise<T>{s}); } - p->fail(s->getFailure()); + p->tryFail(s->getFailure()); } }; state->addChain(std::move(chain)); @@ -770,14 +769,13 @@ inline celix::Promise<U> celix::Promise<void>::then(std::function<celix::Promise auto tmpPromise = success(celix::Promise<void>{s}); p->resolveWith(*tmpPromise.state); } catch (...) { - //failure(); TODO not sure if this needs to be called - p->fail(std::current_exception()); + p->tryFail(std::current_exception()); } } else { if (failure) { failure(celix::Promise<void>{s}); } - p->fail(s->getFailure()); + p->tryFail(s->getFailure()); } }; state->addChain(std::move(chain)); diff --git a/libs/promises/api/celix/impl/SharedPromiseState.h b/libs/promises/api/celix/impl/SharedPromiseState.h index ebe39d7..f92fa05 100644 --- a/libs/promises/api/celix/impl/SharedPromiseState.h +++ b/libs/promises/api/celix/impl/SharedPromiseState.h @@ -46,22 +46,18 @@ namespace celix::impl { ~SharedPromiseState() noexcept = default; - void resolve(T&& value); - - void resolve(const T& value); - template<typename U> void resolveWith(SharedPromiseState<U>& with); - void fail(std::exception_ptr e); - - template<typename E> - void fail(const E &e); + bool tryResolve(T&& value); - bool tryResolve(T &&value); + bool tryResolve(const T& value); bool tryFail(std::exception_ptr e); + template<typename E> + bool tryFail(const E& e); + // copy/move depending on situation T& getValue() &; const T& getValue() const &; @@ -151,17 +147,13 @@ namespace celix::impl { ~SharedPromiseState() noexcept = default; - void resolve(); - - void fail(std::exception_ptr e); - - template<typename E> - void fail(const E &e); - bool tryResolve(); bool tryFail(std::exception_ptr e); + template<typename E> + bool tryFail(const E& e); + bool getValue() const; std::exception_ptr getFailure() const; @@ -278,79 +270,28 @@ inline std::weak_ptr<celix::impl::SharedPromiseState<void>> celix::impl::SharedP } template<typename T> -void celix::impl::SharedPromiseState<T>::resolve(T&& value) { - std::unique_lock<std::mutex> lck{mutex}; - if (done) { - throw celix::PromiseInvocationException("Cannot resolve Promise. Promise is already done"); - } - dataMoved = false; - if constexpr (std::is_move_constructible_v<T>) { - data = std::forward<T>(value); - } else { - data = value; - } - exp = nullptr; - complete(lck); -} - - -template<typename T> -void celix::impl::SharedPromiseState<T>::resolve(const T& value) { - std::unique_lock<std::mutex> lck{mutex}; - if (done) { - throw celix::PromiseInvocationException("Cannot resolve Promise. Promise is already done"); - } - dataMoved = false; - data = value; - exp = nullptr; - complete(lck); -} - -inline void celix::impl::SharedPromiseState<void>::resolve() { - std::unique_lock<std::mutex> lck{mutex}; - if (done) { - throw celix::PromiseInvocationException("Cannot resolve Promise. Promise is already done"); - } - exp = nullptr; - complete(lck); -} - -template<typename T> -void celix::impl::SharedPromiseState<T>::fail(std::exception_ptr e) { - std::unique_lock<std::mutex> lck{mutex}; - if (done) { - throw celix::PromiseInvocationException("Cannot fail Promise. Promise is already done"); - } - exp = std::move(e); - complete(lck); -} - -inline void celix::impl::SharedPromiseState<void>::fail(std::exception_ptr e) { +bool celix::impl::SharedPromiseState<T>::tryResolve(T&& value) { std::unique_lock<std::mutex> lck{mutex}; - if (done) { - throw celix::PromiseInvocationException("Cannot fail Promise. Promise is already done"); + if (!done) { + dataMoved = false; + if constexpr (std::is_move_constructible_v<T>) { + data = std::forward<T>(value); + } else { + data = value; + } + exp = nullptr; + complete(lck); + return true; } - exp = std::move(e); - complete(lck); -} - -template<typename T> -template<typename E> -void celix::impl::SharedPromiseState<T>::fail(const E& e) { - fail(std::make_exception_ptr(e)); -} - -template<typename E> -inline void celix::impl::SharedPromiseState<void>::fail(const E& e) { - fail(std::make_exception_ptr<E>(e)); + return false; } template<typename T> -bool celix::impl::SharedPromiseState<T>::tryResolve(T&& value) { +bool celix::impl::SharedPromiseState<T>::tryResolve(const T& value) { std::unique_lock<std::mutex> lck{mutex}; if (!done) { dataMoved = false; - data = std::forward<T>(value); + data = value; exp = nullptr; complete(lck); return true; @@ -390,6 +331,17 @@ inline bool celix::impl::SharedPromiseState<void>::tryFail(std::exception_ptr e) } template<typename T> +template<typename E> +bool celix::impl::SharedPromiseState<T>::tryFail(const E& e) { + return tryFail(std::make_exception_ptr<E>(e)); +} + +template<typename E> +bool celix::impl::SharedPromiseState<void>::tryFail(const E& e) { + return tryFail(std::make_exception_ptr<E>(e)); +} + +template<typename T> bool celix::impl::SharedPromiseState<T>::isDone() const { std::lock_guard lck{mutex}; return done; @@ -623,14 +575,12 @@ std::shared_ptr<celix::impl::SharedPromiseState<T>> celix::impl::SharedPromiseSt state->scheduledExecutor->schedule(state->priority, duration, [v = std::move(v), e, state] { try { if (v) { - state->resolve(std::move(*v)); + state->tryResolve(std::move(*v)); } else { - state->fail(e); + state->tryFail(e); } - } catch (celix::PromiseInvocationException &) { - //somebody already resolved promise? } catch (...) { - state->fail(std::current_exception()); + state->tryFail(std::current_exception()); } }); }); @@ -644,14 +594,12 @@ std::shared_ptr<celix::impl::SharedPromiseState<void>> celix::impl::SharedPromis state->scheduledExecutor->schedule(state->priority, duration, [e, state] { try { if (!e) { - state->resolve(); + state->tryResolve(); } else { - state->fail(*e); + state->tryFail(*e); } - } catch (celix::PromiseInvocationException &) { - //somebody already resolved promise? } catch (...) { - state->fail(std::current_exception()); + state->tryFail(std::current_exception()); } }); }); @@ -666,12 +614,12 @@ std::shared_ptr<celix::impl::SharedPromiseState<T>> celix::impl::SharedPromiseSt auto p = celix::impl::SharedPromiseState<T>::create(executor, scheduledExecutor, priority); addOnResolve([p, recover = std::move(recover)](std::optional<T> v, const std::exception_ptr& /*e*/) { if (v) { - p->resolve(std::move(*v)); + p->tryResolve(std::move(*v)); } else { try { - p->resolve(recover()); + p->tryResolve(recover()); } catch (...) { - p->fail(std::current_exception()); //or state->failure(); + p->tryFail(std::current_exception()); //or state->failure(); } } }); @@ -687,13 +635,13 @@ inline std::shared_ptr<celix::impl::SharedPromiseState<void>> celix::impl::Share addOnResolve([p, recover = std::move(recover)](std::optional<std::exception_ptr> e) { if (!e) { - p->resolve(); + p->tryResolve(); } else { try { recover(); - p->resolve(); + p->tryResolve(); } catch (...) { - p->fail(std::current_exception()); //or state->failure(); + p->tryFail(std::current_exception()); //or state->failure(); } } }); @@ -710,15 +658,15 @@ std::shared_ptr<celix::impl::SharedPromiseState<T>> celix::impl::SharedPromiseSt if (s->isSuccessfullyResolved()) { try { if (predicate(s->getValue())) { - p->resolve(s->moveOrGetValue()); + p->tryResolve(s->moveOrGetValue()); } else { throw celix::PromiseInvocationException{"predicate does not accept value"}; } } catch (...) { - p->fail(std::current_exception()); + p->tryFail(std::current_exception()); } } else { - p->fail(s->getFailure()); + p->tryFail(s->getFailure()); } }; addChain(std::move(chainFunction)); @@ -731,12 +679,12 @@ std::shared_ptr<celix::impl::SharedPromiseState<T>> celix::impl::SharedPromiseSt auto p = celix::impl::SharedPromiseState<T>::create(executor, scheduledExecutor, priority); auto chainFunction = [s = self.lock(), p, fallbackTo = std::move(fallbackTo)] { if (s->isSuccessfullyResolved()) { - p->resolve(s->moveOrGetValue()); + p->tryResolve(s->moveOrGetValue()); } else { if (fallbackTo->isSuccessfullyResolved()) { - p->resolve(fallbackTo->moveOrGetValue()); + p->tryResolve(fallbackTo->moveOrGetValue()); } else { - p->fail(s->getFailure()); + p->tryFail(s->getFailure()); } } }; @@ -749,13 +697,13 @@ inline std::shared_ptr<celix::impl::SharedPromiseState<void>> celix::impl::Share auto chainFunction = [s = self.lock(), p, fallbackTo = std::move(fallbackTo)] { if (s->isSuccessfullyResolved()) { s->getValue(); - p->resolve(); + p->tryResolve(); } else { if (fallbackTo->isSuccessfullyResolved()) { fallbackTo->getValue(); - p->resolve(); + p->tryResolve(); } else { - p->fail(s->getFailure()); + p->tryFail(s->getFailure()); } } }; @@ -808,12 +756,12 @@ std::shared_ptr<celix::impl::SharedPromiseState<R>> celix::impl::SharedPromiseSt auto chainFunction = [s = self.lock(), p, mapper = std::move(mapper)] { try { if (s->isSuccessfullyResolved()) { - p->resolve(mapper(s->moveOrGetValue())); + p->tryResolve(mapper(s->moveOrGetValue())); } else { - p->fail(s->getFailure()); + p->tryFail(s->getFailure()); } } catch (...) { - p->fail(std::current_exception()); + p->tryFail(std::current_exception()); } }; addChain(std::move(chainFunction)); @@ -830,12 +778,12 @@ std::shared_ptr<celix::impl::SharedPromiseState<R>> celix::impl::SharedPromiseSt try { if (s->isSuccessfullyResolved()) { s->getValue(); - p->resolve(mapper()); + p->tryResolve(mapper()); } else { - p->fail(s->getFailure()); + p->tryFail(s->getFailure()); } } catch (...) { - p->fail(std::current_exception()); + p->tryFail(std::current_exception()); } }; addChain(std::move(chainFunction)); @@ -852,12 +800,12 @@ std::shared_ptr<celix::impl::SharedPromiseState<T>> celix::impl::SharedPromiseSt if (s->isSuccessfullyResolved()) { try { consumer(s->getValue()); - p->resolve(s->moveOrGetValue()); + p->tryResolve(s->moveOrGetValue()); } catch (...) { - p->fail(std::current_exception()); + p->tryFail(std::current_exception()); } } else { - p->fail(s->getFailure()); + p->tryFail(s->getFailure()); } }; addChain(std::move(chainFunction)); @@ -874,12 +822,12 @@ inline std::shared_ptr<celix::impl::SharedPromiseState<void>> celix::impl::Share try { s->getValue(); consumer(); - p->resolve(); + p->tryResolve(); } catch (...) { - p->fail(std::current_exception()); + p->tryFail(std::current_exception()); } } else { - p->fail(s->getFailure()); + p->tryFail(s->getFailure()); } }; addChain(std::move(chainFunction)); diff --git a/libs/promises/gtest/src/PromisesTestSuite.cc b/libs/promises/gtest/src/PromisesTestSuite.cc index 6469a24..7c18de7 100644 --- a/libs/promises/gtest/src/PromisesTestSuite.cc +++ b/libs/promises/gtest/src/PromisesTestSuite.cc @@ -575,7 +575,6 @@ TEST_F(PromiseTestSuite, outOfScopeUnresolvedPromises) { TEST_F(PromiseTestSuite, chainPromises) { auto success = [&](celix::Promise<long> p) -> celix::Promise<long> { - //TODO Promises::resolved(p.getValue() + p.getValue()) auto result = factory->deferred<long>(); result.resolve(p.getValue() + p.getValue()); return result.getPromise(); @@ -668,6 +667,128 @@ TEST_F(PromiseTestSuite, getExecutorFromFactory) { EXPECT_EQ(executor.get(), exec.get()); } +TEST_F(PromiseTestSuite, testRobustFailAndResolve) { + std::atomic<int> failCount{}; + std::atomic<int> successCount{}; + auto failCb = [&failCount](const std::exception& /*e*/) { + failCount++; + }; + auto successCb = [&successCount](int val) { + EXPECT_EQ(42, val); + successCount++; + }; + + auto def = factory->deferred<int>(); + def.getPromise().onFailure(failCb); + + //Rule a second fail should not lead to an exception, to ensure a more robust usage. + //But also should only lead to a single resolve chain. + def.fail(std::logic_error{"error1"}); + def.fail(std::logic_error{"error2"}); + factory->wait(); + EXPECT_EQ(failCount.load(), 1); + + def = factory->deferred<int>(); + def.getPromise().onSuccess(successCb); + //Rule a second resolve should not lead to an exception, to ensure a more robust usage. + //But also should only lead to a single resolve chain. + def.resolve(42); + def.resolve(43); + factory->wait(); + EXPECT_EQ(successCount.load(), 1); + + + failCount = 0; + successCount = 0; + def = factory->deferred<int>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + //Rule a resolve after fail should not lead to an exception, to ensure a more robust usage. + //But also should only lead to a single resolve chain. + def.fail(std::logic_error("error3")); + def.resolve(43); + factory->wait(); + EXPECT_EQ(failCount.load(), 1); + EXPECT_EQ(successCount.load(), 0); + + failCount = 0; + successCount = 0; + def = factory->deferred<int>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + //Rule a fail after resolve should not lead to an exception, to ensure a more robust usage. + //But also should only lead to a single resolve chain. + def.resolve(42); + def.fail(std::logic_error("error3")); + factory->wait(); + EXPECT_EQ(failCount.load(), 0); + EXPECT_EQ(successCount.load(), 1); +} + +TEST_F(PromiseTestSuite, testTryFailAndResolve) { + std::atomic<int> failCount{}; + std::atomic<int> successCount{}; + auto failCb = [&failCount](const std::exception& /*e*/) { + failCount++; + }; + auto successCb = [&successCount](int val) { + EXPECT_EQ(42, val); + successCount++; + }; + const int val = 42; + + //first resolve with &&, then try rest + auto def = factory->deferred<int>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + EXPECT_TRUE(def.tryResolve(42)); + EXPECT_FALSE(def.tryResolve(43)); + EXPECT_FALSE(def.tryFail(std::logic_error{"error"})); + EXPECT_FALSE(def.tryFail(std::make_exception_ptr(std::logic_error{"error"}))); + factory->wait(); + EXPECT_EQ(failCount.load(), 0); + EXPECT_EQ(successCount.load(), 1); + + //first resolve with const int&, then try rest + failCount = 0; + successCount = 0; + def = factory->deferred<int>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + EXPECT_TRUE(def.tryResolve(val)); + EXPECT_FALSE(def.tryResolve(43)); + EXPECT_FALSE(def.tryResolve(val)); + EXPECT_FALSE(def.tryFail(std::logic_error{"error"})); + EXPECT_FALSE(def.tryFail(std::make_exception_ptr(std::logic_error{"error"}))); + factory->wait(); + EXPECT_EQ(failCount.load(), 0); + EXPECT_EQ(successCount.load(), 1); + + //first fail with exp ref, then try rest + failCount = 0; + successCount = 0; + def = factory->deferred<int>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + EXPECT_TRUE(def.tryFail(std::logic_error{"error"})); + EXPECT_FALSE(def.tryResolve(43)); + EXPECT_FALSE(def.tryResolve(val)); + EXPECT_FALSE(def.tryFail(std::logic_error{"error"})); + EXPECT_FALSE(def.tryFail(std::make_exception_ptr(std::logic_error{"error"}))); + factory->wait(); + EXPECT_EQ(failCount.load(), 1); + EXPECT_EQ(successCount.load(), 0); + + //first fail with exp ptr, then try rest + failCount = 0; + successCount = 0; + def = factory->deferred<int>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + EXPECT_TRUE(def.tryFail(std::make_exception_ptr(std::logic_error{"error"}))); + EXPECT_FALSE(def.tryResolve(43)); + EXPECT_FALSE(def.tryResolve(val)); + EXPECT_FALSE(def.tryFail(std::logic_error{"error"})); + EXPECT_FALSE(def.tryFail(std::make_exception_ptr(std::logic_error{"error"}))); + factory->wait(); + EXPECT_EQ(failCount.load(), 1); + EXPECT_EQ(successCount.load(), 0); +} + #ifdef __clang__ #pragma clang diagnostic pop #endif diff --git a/libs/promises/gtest/src/VoidPromisesTestSuite.cc b/libs/promises/gtest/src/VoidPromisesTestSuite.cc index 9accd54..6e25215 100644 --- a/libs/promises/gtest/src/VoidPromisesTestSuite.cc +++ b/libs/promises/gtest/src/VoidPromisesTestSuite.cc @@ -501,6 +501,108 @@ TEST_F(VoidPromiseTestSuite, deferredTaskCall) { EXPECT_GT(durationInMs, std::chrono::milliseconds{10}); } +TEST_F(VoidPromiseTestSuite, testRobustFailAndResolve) { + std::atomic<int> failCount{}; + std::atomic<int> successCount{}; + auto failCb = [&failCount](const std::exception& /*e*/) { + failCount++; + }; + auto successCb = [&successCount]() { + successCount++; + }; + + auto def = factory->deferred<void>(); + def.getPromise().onFailure(failCb); + + //Rule a second fail should not lead to an exception, to ensure a more robust usage. + //But also should only lead to a single resolve chain. + def.fail(std::logic_error{"error1"}); + def.fail(std::logic_error{"error2"}); + factory->wait(); + EXPECT_EQ(failCount.load(), 1); + + def = factory->deferred<void>(); + def.getPromise().onSuccess(successCb); + //Rule a second resolve should not lead to an exception, to ensure a more robust usage. + //But also should only lead to a single resolve chain. + def.resolve(); + def.resolve(); + factory->wait(); + EXPECT_EQ(successCount.load(), 1); + + + failCount = 0; + successCount = 0; + def = factory->deferred<void>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + //Rule a resolve after fail should not lead to an exception, to ensure a more robust usage. + //But also should only lead to a single resolve chain. + def.fail(std::logic_error("error3")); + def.resolve(); + factory->wait(); + EXPECT_EQ(failCount.load(), 1); + EXPECT_EQ(successCount.load(), 0); + + failCount = 0; + successCount = 0; + def = factory->deferred<void>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + //Rule a fail after resolve should not lead to an exception, to ensure a more robust usage. + //But also should only lead to a single resolve chain. + def.resolve(); + def.fail(std::logic_error("error3")); + factory->wait(); + EXPECT_EQ(failCount.load(), 0); + EXPECT_EQ(successCount.load(), 1); +} + +TEST_F(VoidPromiseTestSuite, testTryFailAndResolve) { + std::atomic<int> failCount{}; + std::atomic<int> successCount{}; + auto failCb = [&failCount](const std::exception& /*e*/) { + failCount++; + }; + auto successCb = [&successCount]() { + successCount++; + }; + + //first resolve, then try rest + auto def = factory->deferred<void>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + EXPECT_TRUE(def.tryResolve()); + EXPECT_FALSE(def.tryFail(std::logic_error{"error"})); + EXPECT_FALSE(def.tryFail(std::make_exception_ptr(std::logic_error{"error"}))); + factory->wait(); + EXPECT_EQ(failCount.load(), 0); + EXPECT_EQ(successCount.load(), 1); + + //first fail with exp ref, then try rest + failCount = 0; + successCount = 0; + def = factory->deferred<void>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + EXPECT_TRUE(def.tryFail(std::logic_error{"error"})); + EXPECT_FALSE(def.tryResolve()); + EXPECT_FALSE(def.tryFail(std::logic_error{"error"})); + EXPECT_FALSE(def.tryFail(std::make_exception_ptr(std::logic_error{"error"}))); + factory->wait(); + EXPECT_EQ(failCount.load(), 1); + EXPECT_EQ(successCount.load(), 0); + + //first fail with exp ptr, then try rest + failCount = 0; + successCount = 0; + def = factory->deferred<void>(); + def.getPromise().onSuccess(successCb).onFailure(failCb); + EXPECT_TRUE(def.tryFail(std::make_exception_ptr(std::logic_error{"error"}))); + EXPECT_FALSE(def.tryResolve()); + EXPECT_FALSE(def.tryFail(std::logic_error{"error"})); + EXPECT_FALSE(def.tryFail(std::make_exception_ptr(std::logic_error{"error"}))); + factory->wait(); + EXPECT_EQ(failCount.load(), 1); + EXPECT_EQ(successCount.load(), 0); +} + #ifdef __clang__ #pragma clang diagnostic pop #endif
