Cluster LeaveAsync() with CancellationToken support #2501

kostrse · 2017-02-04T13:24:58Z

I added overload of the LeaveAsync() method with support of CancellationToken.

Cancellation token support should be useful assurance to avoid unexpected hang of application if member removal confirmation didn't come for some reason.

I also made the leave logic by LeaveAsync() to be triggered only once (via Interlocked). This is probably not critical for the cluster daemon, but just in case 😺 (related to #2498)

Usage example:

    var cts = new CancellationTokenSource();
    cts.CancelAfter(TimeSpan.FromMinutes(5));

    await Cluster.Get(_actorSystem).LeaveAsync(cancellationToken);
    await _actorSystem.Terminate();

Related issues: #2280, #2498

Also:

Leave() and LeaveAsync() do not cooperate now, but it's possible to make Leave() call LeaveSelf() to share state between them:

public void Leave(Address address)
{
    if (address == SelfAddress)
    {
        LeaveSelf();
    }
    else
        ClusterCore.Tell(new ClusterUserAction.Leave(FillLocal(address)));
    }
}

Aaronontheweb

Left some questions, but overall I think these changes are good.

Aaronontheweb · 2017-02-04T18:39:21Z

src/core/Akka.Cluster.Tests/ClusterSpec.cs

@@ -192,6 +194,76 @@ public void A_cluster_must_complete_LeaveAsync_task_upon_being_removed()
            _cluster.LeaveAsync().IsCompleted.Should().BeTrue();
        }

+        [Fact(Skip = "This behavior not yet supported.")]


Why isn't this behavior supported yet?

Because wanted to discuss before 😄

Right now Leave() and LeaveAsync() do not share state with each other.

LeaveAsync() memoizes the _leaveTask, therefore two subsequent calls of LeaveAsync() won't result in double sending of the leave request.

But Leave() doesn't take the _leaveTask into account. So, if cluster have been left by the Leave() method (or is in state of leaving) , LeaveAsync() doesn't know about it and will be waiting for the MemberRemoved event.

Here is how we can change the Leave() method to support that:

public void Leave(Address address) { if (address == SelfAddress) { LeaveSelf(); } else ClusterCore.Tell(new ClusterUserAction.Leave(FillLocal(address))); }

I just wanted to check before, to be sure that it won't make any negative effect on anything.
Plus the self address comparison looks questionable.

Might need to compare the output of FillLocal(address) to SelfAddress, although I'd need to look at the FillLocal function again to say for that sure :p

Aaronontheweb · 2017-02-04T18:39:51Z

src/core/Akka.Cluster/Cluster.cs

-            if (tcs.Task.IsCompleted)
-                return tcs.Task;
+            // It's assumed here that once the member left the cluster, it won't get back again.
+            // So, the member removal event being memoized in TaskCompletionSource and never reset.


Makes sense

Aaronontheweb · 2017-02-04T18:41:23Z

src/core/Akka/Util/Internal/TaskExtensions.cs

+        /// <param name="task">The original task.</param>
+        /// <param name="cancellationToken">The cancellation token.</param>
+        /// <returns>The task which completes with result of original task or with cancelled state.</returns>
+        public static Task WithCancellation(this Task task, CancellationToken cancellationToken)


Nice, I like it, although maybe we should mark this as internal? cc @alexvaluyskiy

The TaskExtensions and Cluster are in different assemblies.
I found this extensions class and decided to add the WithCancellation() method here, but I can move it into another place if you know a better location.

Is the name WithCancellation sounds good, BTW?

@kostrse actually it's probably fine to leave this public. WithCancellation sounds great 👍

The TaskExtensions and Cluster are in different assemblies.

We are using [InternalVisibleToAttribute]. So, It does not matter that these classes are situated in different assemblies

maxim-s · 2017-02-06T05:17:56Z

src/core/Akka.Cluster/Cluster.cs

+        private Task LeaveSelf()
+        {
+            var tcs = new TaskCompletionSource<object>();
+            var leaveTask = Interlocked.CompareExchange(ref _leaveTask, tcs.Task, null);


Nice solution

Danthar · 2017-02-07T10:33:24Z

Note: windows-unit-tests are failing due to Api Approval.

Aaronontheweb · 2017-02-07T16:55:43Z

@kostrse for fixing the public API issue: http://getakka.net/docs/akka-developers/public-api-changes

kostrse · 2017-02-09T00:20:15Z

Thanks, will finalize the code review soon.

…

----- Original message ----- From: Aaron Stannard <notifications@github.com> To: "akkadotnet/akka.net" <akka.net@noreply.github.com> Cc: Sergey Kostrukov <kostrse@gmail.com>, Mention <mention@noreply.github.com> Subject: Re: [akkadotnet/akka.net] Cluster LeaveAsync() with CancellationToken support (#2501) Date: Tue, 07 Feb 2017 08:55:46 -0800 @kostrse[1] for fixing the public API issue: http://getakka.net/docs/akka-developers/public-api-changes — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub[2], or mute the thread[3]. Links: 1. https://github.com/kostrse 2. #2501 (comment) 3. https://github.com/notifications/unsubscribe-auth/ABFC9a8796xcTUTamCjps2UQDgspC2Blks5raKISgaJpZM4L3Jmm

Aaronontheweb · 2017-02-14T01:28:38Z

@kostrse have you had a chance to update this yet?

kostrse · 2017-02-14T02:09:36Z

Sorry for delay, working on it now

kostrse · 2017-02-14T03:11:05Z

The test ClusterSpec.A_cluster_must_complete_LeaveAsync_task_upon_being_removed is still racy:

Timeout 00:00:10 expired while waiting for condition.Failed to observe node as Exiting.
Expected: True
Actual:   False
   at Akka.TestKit.Xunit2.XunitAssertions.Fail(String format, Object[] args) in D:\work\d26c9d7f36545acd\src\contrib\testkits\Akka.TestKit.Xunit2\XunitAssertions.cs:line 27
   at Akka.TestKit.TestKitBase.<>c__DisplayClass135_0.<AwaitCondition>b__0(String format, Object[] args) in D:\work\d26c9d7f36545acd\src\core\Akka.TestKit\TestKitBase_AwaitConditions.cs:line 91
   at Akka.TestKit.TestKitBase.InternalAwaitCondition(Func`1 conditionIsFulfilled, TimeSpan max, Nullable`1 interval, Action`2 fail, ILoggingAdapter logger) in D:\work\d26c9d7f36545acd\src\core\Akka.TestKit\TestKitBase_AwaitConditions.cs:line 229
   at Akka.TestKit.TestKitBase.AwaitCondition(Func`1 conditionIsFulfilled, Nullable`1 max, String message) in D:\work\d26c9d7f36545acd\src\core\Akka.TestKit\TestKitBase_AwaitConditions.cs:line 92
   at Akka.Cluster.Tests.ClusterSpec.<>c__DisplayClass16_0.<A_cluster_must_complete_LeaveAsync_task_upon_being_removed>b__2() in D:\work\d26c9d7f36545acd\src\core\Akka.Cluster.Tests\ClusterSpec.cs:line 187
   at Akka.TestKit.TestKitBase.<>c__DisplayClass116_0.<Within>b__0() in D:\work\d26c9d7f36545acd\src\core\Akka.TestKit\TestKitBase_Within.cs:line 48
   at Akka.TestKit.TestKitBase.Within[T](TimeSpan min, TimeSpan max, Func`1 function, String hint, Nullable`1 epsilonValue) in D:\work\d26c9d7f36545acd\src\core\Akka.TestKit\TestKitBase_Within.cs:line 101
   at Akka.TestKit.TestKitBase.Within(TimeSpan min, TimeSpan max, Action action, String hint, Nullable`1 epsilonValue) in D:\work\d26c9d7f36545acd\src\core\Akka.TestKit\TestKitBase_Within.cs:line 49
   at Akka.TestKit.TestKitBase.Within(TimeSpan max, Action action, Nullable`1 epsilonValue) in D:\work\d26c9d7f36545acd\src\core\Akka.TestKit\TestKitBase_Within.cs:line 32
   at Akka.Cluster.Tests.ClusterSpec.A_cluster_must_complete_LeaveAsync_task_upon_being_removed() in D:\work\d26c9d7f36545acd\src\core\Akka.Cluster.Tests\ClusterSpec.cs:line 193

I don't know yet the TestKit very well to track down the problem :(

Aaronontheweb · 2017-02-20T18:46:32Z

@kostrse I think #2516 needs to be added in order to formally solve that problem.

Aaronontheweb · 2017-03-14T15:28:48Z

@kostrse would you mind handling the merge conflict here?

kostrse · 2017-03-23T08:56:33Z

Yes, sorry, will take a look now.

…llation

kostrse · 2017-03-23T10:38:22Z

the continuous-integration/nuget-pack build failure probably not related to the change itself.

kostrse and others added 3 commits February 4, 2017 20:54

Cluster LeaveAsync() with CancellationToken

80a3692

Merge branch 'dev' into dev-leaveasync-cancellation

27ab5f3

Merge branch 'dev' into dev-leaveasync-cancellation

e2c1d4a

Aaronontheweb reviewed Feb 4, 2017

View reviewed changes

maxim-s reviewed Feb 6, 2017

View reviewed changes

maxim-s mentioned this pull request Feb 6, 2017

[WIP] possible fix for #2492 #2498

Merged

Merge branch 'dev' into dev-leaveasync-cancellation

4ab859a

Aaronontheweb and others added 2 commits February 10, 2017 11:56

Merge branch 'dev' into dev-leaveasync-cancellation

a8e2eb3

Merge branch 'dev' into dev-leaveasync-cancellation

482d58f

kostrse added 3 commits February 14, 2017 10:13

Merge branch 'dev' into dev-leaveasync-cancellation

b8bbed6

LeaveAsync() doesn't trigger another leave after Leave()

29c6012

APi changes

0526f91

Merge branch 'dev' into dev-leaveasync-cancellation

9f0008e

Aaronontheweb added this to the 1.2.0 milestone Feb 20, 2017

Merge remote-tracking branch 'upstream/dev' into dev-leaveasync-cance…

70d8759

…llation

Aaronontheweb merged commit 69baba6 into akkadotnet:dev Mar 25, 2017

kostrse deleted the dev-leaveasync-cancellation branch March 25, 2017 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster LeaveAsync() with CancellationToken support #2501

Cluster LeaveAsync() with CancellationToken support #2501

kostrse commented Feb 4, 2017 •

edited

Loading

Aaronontheweb left a comment

Aaronontheweb Feb 4, 2017

kostrse Feb 4, 2017

Aaronontheweb Feb 10, 2017

Aaronontheweb Feb 4, 2017

Aaronontheweb Feb 4, 2017

kostrse Feb 4, 2017

Aaronontheweb Feb 7, 2017

alexvaluyskiy Feb 16, 2017 •

edited

Loading

maxim-s Feb 6, 2017

Danthar commented Feb 7, 2017

Aaronontheweb commented Feb 7, 2017

kostrse commented Feb 9, 2017 via email

Aaronontheweb commented Feb 14, 2017

kostrse commented Feb 14, 2017

kostrse commented Feb 14, 2017

Aaronontheweb commented Feb 20, 2017

Aaronontheweb commented Mar 14, 2017

kostrse commented Mar 23, 2017

kostrse commented Mar 23, 2017

Cluster LeaveAsync() with CancellationToken support #2501

Cluster LeaveAsync() with CancellationToken support #2501

Conversation

kostrse commented Feb 4, 2017 • edited Loading

Usage example:

Also:

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexvaluyskiy Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danthar commented Feb 7, 2017

Aaronontheweb commented Feb 7, 2017

kostrse commented Feb 9, 2017 via email

Aaronontheweb commented Feb 14, 2017

kostrse commented Feb 14, 2017

kostrse commented Feb 14, 2017

Aaronontheweb commented Feb 20, 2017

Aaronontheweb commented Mar 14, 2017

kostrse commented Mar 23, 2017

kostrse commented Mar 23, 2017

kostrse commented Feb 4, 2017 •

edited

Loading

alexvaluyskiy Feb 16, 2017 •

edited

Loading