Recover continuable after invoking void fail handlers, s.t. continuab…

yaoyuan1216 commented 2 years ago

Solve the second case of issue #46.

@Naios

What was a problem?

In previous revision (modified by 7a5bde32), a continuable is considered completed and will not invoke subsequent handlers after a void fail handler is invoked, as a result it will be blocked if the continuable is waited.

How this PR fixes the problem?

Invoke subsequent handlers after void fail handlers invoked.

Check lists (check `x` in `[ ]` of list items)

[x] Additional Unit Tests were added that test the feature or regression
[ ] Coding style (Clang format was applied)

Naios commented 2 years ago

Thanks for your PR.

I think a major issue of your PR is that it changes the meaning of recovery and fail handlers completely:

Currently a continuation chain is not continued after a fail handler is invoked. This makes it possible to perform scoped error handling:
```
.then(_) // 1
.then(_) // 2
.fail(_) // Handles the error of 1 & 2
.then(_) // 3
.fail(_); // Only handles the error of 3 
```
Therefore your changes would break a existing code relying on this behaviour.
In order to recover from a failure handler to return a valid result, cti::recover(_) can be used to continue the continuation on a valid path (it leads to the same result as you achieved by the changes made in your PR, but explicitly).

I think a much better way in solving this issue is to call the cancellation handler after a handled failure (1). The case of cancellation should already be handled in the handler for wait, and correctly unlock the mutex.

Another possibility is to guard the wait handler through a RAII object that automatically unlocks the mutex on destruction (2) (if not previously unlocked).

Probably solution (1) is preferrable over (2).

yaoyuan1216 commented 2 years ago

If I understand correctly, you would expect the following code to work?

TYPED_TEST(single_dimension_tests, then_after_nonvoid_fail_handler_invoked) {
   bool invoked = false;
   make_exceptional_continuable<int>(supply_test_exception())
                    .fail([]{
                      return 1;
                    })
                    .then([&]{
                      EXPECT_FALSE(invoked);
                      invoked = true;
                    });

   ASSERT_TRUE(invoked);
 }

 TYPED_TEST(single_dimension_tests, then_after_void_fail_handler_invoked) {
   bool invoked = false;
   make_exceptional_continuable<void>(supply_test_exception())
                    .fail([]{
                      return;
                    })
                    .then([&]{
                      EXPECT_FALSE(invoked);
                      invoked = true;
                    });

   ASSERT_TRUE(invoked);
 }

TYPED_TEST(single_dimension_tests, scoped_fail_handler_not_invoked) {
   bool invoked1 = false;
   bool invoked2 = false;
   make_exceptional_continuable<void>(supply_test_exception())
                    .fail([&]{
                      EXPECT_FALSE(invoked1);
                      invoked1 = true;
                      return;
                    })
                    .then([]{
                      return;
                    })
                    .fail([&]{
                      EXPECT_FALSE(invoked2);
                      invoked2 = true;
                    });

   ASSERT_TRUE(invoked1);
   ASSERT_FALSE(invoked2);
 }

 TYPED_TEST(single_dimension_tests, scoped_fail_handler_invoked) {
   bool invoked1 = false;
   bool invoked2 = false;
   make_exceptional_continuable<void>(supply_test_exception())
                    .fail([&]{
                      EXPECT_FALSE(invoked1);
                      invoked1 = true;
                      return;
                    })
                    .then([]{
                      std::rethrow_exception(supply_test_exception());
                    })
                    .fail([&]{
                      EXPECT_FALSE(invoked2);
                      invoked2 = true;
                    });

   ASSERT_TRUE(invoked1);
   ASSERT_TRUE(invoked2);
 }

However, then_after_void_fail_handler_invoked, scoped_fail_handler_invoked will fail with current revision. With my PR, all above code will succeed. In regard to recover and cancel, I think cancel should be explicit and recover should be implicit. In regard to the wait problem, it is not caused by the mutex lock issue. The problem is next handler not called after void fail handler invoked, which is not the case with non-void fail handler. And it is the fatal problem I fixed.

Naios commented 2 years ago

I fixed this issue in https://github.com/Naios/continuable/commit/b51be39e7126c397cec9d13fefdfa3cbbe8291f0 in a different way that doesn't alter the libraries behaviour. Thank you for your PR, although I could not accept it,

Naios / continuable