You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2021/10/05 17:18:39 UTC

[GitHub] [celix] pnoltes opened a new pull request #370: Updates Promise `resolve` and `fail` functions to make them robust.

pnoltes opened a new pull request #370:
URL: https://github.com/apache/celix/pull/370


   This PR makes the `celix::Deferred<T>::fail` and `celix::Deferred<T>::resolve` robust for resolving a  promise if it is already resolved. 
   
   This is different from the OSGi spec and this is done  because it always a race condition to check if a promise is already resolved (`isDone()`) and then resolve the promise. 
   
   The methods `celix::Deferred<T>::tryFail` and `celix::Deferred<T>::tryResolve` are added to resolve a promise and check if it was already resolved atomically.  


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] pnoltes commented on a change in pull request #370: Updates Promise `resolve` and `fail` functions to make them robust.

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #370:
URL: https://github.com/apache/celix/pull/370#discussion_r723005705



##########
File path: libs/promises/api/celix/Deferred.h
##########
@@ -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>>

Review comment:
       if a function is a template function, it is already inline. So only the function with a template arg ('normal' functions or template specialized functIon) need a inline




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] codecov-commenter commented on pull request #370: Updates Promise `resolve` and `fail` functions to make them robust.

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #370:
URL: https://github.com/apache/celix/pull/370#issuecomment-934617523


   # [Codecov](https://codecov.io/gh/apache/celix/pull/370?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#370](https://codecov.io/gh/apache/celix/pull/370?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4329677) into [master](https://codecov.io/gh/apache/celix/commit/7250126d92110d9b1d9fc5272c67fdb557dac49a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7250126) will **decrease** coverage by `0.00%`.
   > The diff coverage is `68.91%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/celix/pull/370/graphs/tree.svg?width=650&height=150&src=pr&token=JdsiThga8P&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/celix/pull/370?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #370      +/-   ##
   ==========================================
   - Coverage   71.59%   71.59%   -0.01%     
   ==========================================
     Files         203      203              
     Lines       34967    34954      -13     
   ==========================================
   - Hits        25036    25025      -11     
   + Misses       9931     9929       -2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/celix/pull/370?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [libs/promises/api/celix/Promise.h](https://codecov.io/gh/apache/celix/pull/370/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9wcm9taXNlcy9hcGkvY2VsaXgvUHJvbWlzZS5o) | `96.15% <50.00%> (ø)` | |
   | [libs/promises/api/celix/impl/SharedPromiseState.h](https://codecov.io/gh/apache/celix/pull/370/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9wcm9taXNlcy9hcGkvY2VsaXgvaW1wbC9TaGFyZWRQcm9taXNlU3RhdGUuaA==) | `86.48% <56.25%> (+1.00%)` | :arrow_up: |
   | [libs/promises/api/celix/Deferred.h](https://codecov.io/gh/apache/celix/pull/370/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9wcm9taXNlcy9hcGkvY2VsaXgvRGVmZXJyZWQuaA==) | `100.00% <100.00%> (ø)` | |
   | [...n\_websocket/v2/src/pubsub\_websocket\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/370/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3dlYnNvY2tldC92Mi9zcmMvcHVic3ViX3dlYnNvY2tldF90b3BpY19zZW5kZXIuYw==) | `84.51% <0.00%> (-2.59%)` | :arrow_down: |
   | [.../pubsub\_admin\_tcp/v2/src/pubsub\_tcp\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/370/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3RjcC92Mi9zcmMvcHVic3ViX3RjcF90b3BpY19zZW5kZXIuYw==) | `84.88% <0.00%> (-0.89%)` | :arrow_down: |
   | [libs/utils/src/hash\_map.c](https://codecov.io/gh/apache/celix/pull/370/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy91dGlscy9zcmMvaGFzaF9tYXAuYw==) | `91.01% <0.00%> (-0.85%)` | :arrow_down: |
   | [.../pubsub\_admin\_zmq/v2/src/pubsub\_zmq\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/370/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3ptcS92Mi9zcmMvcHVic3ViX3ptcV90b3BpY19zZW5kZXIuYw==) | `88.18% <0.00%> (+1.26%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/370?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/370?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [7250126...4329677](https://codecov.io/gh/apache/celix/pull/370?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] pnoltes merged pull request #370: Updates Promise `resolve` and `fail` functions to make them robust.

Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #370:
URL: https://github.com/apache/celix/pull/370


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] stegemr commented on a change in pull request #370: Updates Promise `resolve` and `fail` functions to make them robust.

Posted by GitBox <gi...@apache.org>.
stegemr commented on a change in pull request #370:
URL: https://github.com/apache/celix/pull/370#discussion_r722595747



##########
File path: libs/promises/api/celix/Deferred.h
##########
@@ -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>>

Review comment:
       no all methods are inline




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] stegemr commented on a change in pull request #370: Updates Promise `resolve` and `fail` functions to make them robust.

Posted by GitBox <gi...@apache.org>.
stegemr commented on a change in pull request #370:
URL: https://github.com/apache/celix/pull/370#discussion_r722599937



##########
File path: libs/promises/gtest/src/PromisesTestSuite.cc
##########
@@ -668,6 +667,128 @@ TEST_F(PromiseTestSuite, getExecutorFromFactory) {
     EXPECT_EQ(executor.get(), exec.get());
 }
 
+TEST_F(PromiseTestSuite, testRobustFailAndResolve) {

Review comment:
       Nice to see the tests!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org