Ask interface should be clean #3220

maxcherednik · 2017-12-21T08:32:46Z

This closes #3162, closes #3182
Tests should be precise - in terms of what to expect

It should return a result if Success
It should throw AskTimeoutException if we didn't get the result witin timeout specified
It MIGHT throw TaskCancelledException if we cancel the operation

Plus: we should use the power of the C# itself. Don't block on async functions - .Result and .Wait() are not cool.

maxcherednik · 2017-12-21T08:37:32Z

src/core/Akka.Tests/Actor/AskSpec.cs

            var actor = Sys.ActorOf<SomeActor>();
-            var cts = new CancellationTokenSource(TimeSpan.FromSeconds(3));
-            Assert.Throws<AggregateException>(() => { actor.Ask<string>("timeout", Timeout.InfiniteTimeSpan, cts.Token).Wait(); });
-            Assert.True(cts.IsCancellationRequested);


No need to test CancellationToken source here - it working just fine.

maxcherednik · 2017-12-21T08:38:44Z

src/core/Akka.Tests/Actor/AskSpec.cs

-            Assert.True(cts.IsCancellationRequested);
+            using (var cts = new CancellationTokenSource(TimeSpan.FromSeconds(3)))
+            {
+                await Assert.ThrowsAsync<TaskCanceledException>(async () => await actor.Ask<string>("timeout", Timeout.InfiniteTimeSpan, cts.Token));


No mess with the AggregateExceptions and unwrapping... clear well defined expectation!

Horusiath · 2017-12-21T08:39:26Z

@maxcherednik we don't use async tests methods, because they introduced races when the test suite has been run in parallel mode.

maxcherednik · 2017-12-21T08:41:25Z

src/core/Akka.Tests/Actor/AskSpec.cs

+            {
+                await Assert.ThrowsAsync<TaskCanceledException>(async () => await actor.Ask<string>("cancel", TimeSpan.FromSeconds(30), cts.Token));
+            }
+
            Are_Temp_Actors_Removed(actor);


@Aaronontheweb @Horusiath What is this thing doing here?
We kinda testing Ask here. As far as I can see it should not affect an actor it's used upon.

Internally in order to work Ask makes a lightweight temporary actor reference. One of the bugs in the past was that those temporary actors where not recycled after Ask task completed, causing memory leaks.

maxcherednik · 2017-12-21T08:43:42Z

@Horusiath hm... and the reason is ? Cause semantically they are the same.

Horusiath · 2017-12-21T10:19:23Z

@maxcherednik the problem is not in semantics, but in internals of xunit test runner, which works (or at least it worked in the past) in mysterious ways and did weird things with scheduling tests to be executed. While you won't feel that in simple unit tests, if you do integration tests (which are most of the Akka.NET test suite) it resulted in false negatives, since often assertions were running into timeouts.

maxcherednik · 2017-12-21T12:22:01Z

@Horusiath probably long in the past. We use them all the time all over. Never had any issues of such nature. I thought it's related to Akka.net implementation or something.

So here specifically can we proceed with the modern way?

Aaronontheweb · 2017-12-21T13:58:33Z

Looks like AskSpec.Can_get_timeout_when_asking_actor failed on all 3 runtimes.

maxcherednik · 2017-12-21T15:29:35Z

@Aaronontheweb I know, that's the purpose. The problem here is that we were never throwing the AskTimeoutException. Instead we were throwing the TaskCancelledException, which is not really clear.
I've put the info in the description what the interface of the Ask method should be.
If we fix this - it's definitely a breaking change. Question - do we want to fix it to make it beautiful(I actually need it in the context of clear shutdown of the Remoting)?

maxcherednik · 2017-12-21T15:32:09Z

src/core/Akka.Tests/Actor/AskSpec.cs

        {
            var actor = Sys.ActorOf<SomeActor>();
-            Assert.Throws<AggregateException>(() => { actor.Ask<string>("timeout", TimeSpan.FromSeconds(3)).Wait(); });


Here because we didn't use the async await syntax - we started to catch AggregateException and never actually checked the underneath type. And this probably caused us picking the wrong type of exception to throw(TaskCancelledException instead of AskTimeoutException)

@Aaronontheweb Look at the code above. It's not actually "to be modern", it's just to help not to hide things.

maxcherednik · 2017-12-21T15:35:17Z

@Aaronontheweb @Horusiath So if we agree on the interface of the Ask method, I will satisfy the failing test by implementing the right way. Shall I?

Aaronontheweb · 2017-12-21T15:49:14Z

I agree with:

It should return a result if Success
It should throw AskTimeoutException if we didn't get the result witin timeout specified
It MIGHT throw TaskCancelledException if we cancel the operation

Don't agree with

Don't block on async functions - .Result and .Wait() are not cool.

Doesn't matter if we block in the test suites or not. As @Horusiath mentioned, we had historical reasons for not doing this in the past. There's no benefit to changing it for the sake of "making it modern." Unless there's a compelling benefit to change something, don't do it.

Aaronontheweb · 2017-12-21T16:05:19Z

Go forward with your changes @maxcherednik - makes sense.

maxcherednik · 2017-12-27T14:23:22Z

src/contrib/cluster/Akka.Cluster.Sharding.Tests.MultiNode/AsyncWriteProxyEx.cs

        {
            IActorRefProvider provider = ResolveProvider(self);
            if (provider == null)
                throw new ArgumentException("Unable to resolve the target Provider", nameof(self));

-            return AskEx(self, messageFactory, provider, timeout, cancellationToken).CastTask<object, T>();
+            return (T)await AskEx(self, messageFactory, provider, timeout, cancellationToken);


We don't need this CastTask at all. All we need to do here is to take await and cast the value from object to T.

The rest will be managed by the .net itself. All the exceptions will be propagated as is.

maxcherednik · 2017-12-27T14:27:29Z

src/contrib/cluster/Akka.Cluster.Sharding.Tests.MultiNode/AsyncWriteProxyEx.cs

@@ -410,49 +410,60 @@ internal static IActorRefProvider ResolveProvider(ICanTell self)
            return null;
        }

-        private static Task<object> AskEx(ICanTell self, Func<IActorRef, object> messageFactory, IActorRefProvider provider, TimeSpan? timeout, CancellationToken cancellationToken)
+        private static async Task<object> AskEx(ICanTell self, Func<IActorRef, object> messageFactory, IActorRefProvider provider, TimeSpan? timeout, CancellationToken cancellationToken)


In this method I did a few changes:

Setting the right exception in case of Timeout

With the help of the async/await - removing one level of indirection(unregister callback is gone) - this logic happens in the try/finaly

Right order of disposing things in the finally block

maxcherednik · 2017-12-27T14:29:47Z

src/core/Akka.Tests/Actor/AskSpec.cs

@@ -123,6 +123,15 @@ protected override void OnReceive(object message)
            Are_Temp_Actors_Removed(actor);
        }

+        [Fact]
+        public async Task ShouldFailWhenAskExpectsWrongType()


Extra test for InvalidCastException

maxcherednik · 2017-12-27T14:31:21Z

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

-        /// <typeparam name="TResult">TBD</typeparam>
-        /// <param name="task">TBD</param>
-        /// <returns>TBD</returns>
-        public static Task<TResult> CastTask<TTask, TResult>(this Task<TTask> task)


Let's not use this guy anymore, but the features of the C# itself.

maxcherednik · 2017-12-27T14:33:17Z

src/core/Akka/Actor/ActorRef.cs

@@ -76,30 +76,29 @@ public class FutureActorRef : MinimalActorRef
        /// TBD


This class is simplified-the only purpose of this class now is to listen for the response message and put it into the completion source. Less callbacks, more readable code.

maxcherednik · 2017-12-27T16:00:30Z

@Horusiath @Aaronontheweb got a q.
Why do we need an overload of Task<object> Ask when we have Task<T> Ask<T> which covers the first one? Shall we push the user of this interface to explicitly say what he expects to receive?

Aaronontheweb · 2017-12-27T18:26:07Z

@maxcherednik

In general we follow extend-only design principles, so we don't remove stuff once it's been added to the API. Not 100&% always, but usually. I'd just leave the Task<object> overload alone.

maxcherednik · 2017-12-27T18:41:25Z

@Aaronontheweb ok, got it. I don't insist, it's just redundant code with zero value. Was just wondering if there were other hidden design reasons to have it.

Aaronontheweb · 2017-12-27T19:07:11Z

I don't insist, it's just redundant code with zero value.

Someone added it a while back and I probably should have said "no" at the time.

maxcherednik · 2017-12-27T19:15:11Z

@Aaronontheweb or @Horusiath could you please review the approach taken and if it's fine, I will add more documentation around Ask interface.

to11mtm · 2018-01-01T01:43:00Z

If the ask method were made async, would that cause hazards for anyone currently using Ask outside of an asynchronous method?

maxcherednik · 2018-01-01T17:02:14Z

@to11mtm the signature of the method stays the same - that is what's important. The rest is implementation details! We will try to implement it not to break current usages.

AskSpecs consolidated Api change approval - internal CastTask removed

maxcherednik · 2018-01-12T15:37:49Z

@Aaronontheweb @Horusiath Could you please review the PR

Aaronontheweb · 2018-01-12T17:39:47Z

@maxcherednik sure thing Maxim; sorry for the delay. Had my hands full debugging cluster stuff over the past couple of days!

Aaronontheweb

Some minor changes needed. Flagged them in latest review.

Aaronontheweb · 2018-01-12T18:40:38Z

src/core/Akka.Tests/Actor/AskSpec.cs

            var actor = Sys.ActorOf<SomeActor>();
-            var cts = new CancellationTokenSource(TimeSpan.FromSeconds(3));
-            Assert.Throws<AggregateException>(() => { actor.Ask<string>("timeout", Timeout.InfiniteTimeSpan, cts.Token).Wait(); });
-            Assert.True(cts.IsCancellationRequested);


Aaronontheweb · 2018-01-12T18:47:59Z

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

@@ -0,0 +1,72 @@
+//-----------------------------------------------------------------------
+// <copyright file="ArrayExtensions.cs" company="Akka.NET Project">


Filename is wrong @maxcherednik

Aaronontheweb · 2018-01-12T18:51:51Z

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

+            return this;
+        }
+
+        public void GetResult()


Is there meant to be something here?

Nah... this method there is to make this class awaitable.

Aaronontheweb · 2018-01-12T18:52:12Z

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

-        /// <typeparam name="TResult">TBD</typeparam>
-        /// <param name="task">TBD</param>
-        /// <returns>TBD</returns>
-        public static Task<TResult> CastTask<TTask, TResult>(this Task<TTask> task)


Aaronontheweb · 2018-01-12T18:53:05Z

src/core/Akka/Routing/ScatterGatherFirstCompleted.cs


+        private async Task<object> SendMessage(object message)


I like the redesign of this class. Much more understandable.

Aaronontheweb · 2018-01-12T19:05:53Z

One thing I really like about this PR is the performance improvement. I want to see more data from our build server to support this, but....

Before

Totals

Metric	Units	Max	Average	Min	StdDev
TotalBytesAllocated	bytes	6,212,320.00	6,176,605.33	6,129,816.00	42,352.26
[Counter] AskReplies	operations	62,455.00	62,455.00	62,455.00	0.00

Per-second Totals

Metric	Units / s	Max / s	Average / s	Min / s	StdDev / s
TotalBytesAllocated	bytes	11,172,576.10	8,357,304.18	5,423,849.57	2,876,184.89
[Counter] AskReplies	operations	113,834.29	84,595.72	54,745.32	29,549.23

After

Totals

Metric	Units	Max	Average	Min	StdDev
TotalBytesAllocated	bytes	3,924,808.00	3,917,082.67	3,903,608.00	11,711.15
[Counter] AskReplies	operations	115,059.00	115,059.00	115,059.00	0.00

Per-second Totals

Metric	Units / s	Max / s	Average / s	Min / s	StdDev / s
TotalBytesAllocated	bytes	4,244,555.46	4,162,325.65	4,121,139.90	71,213.14
[Counter] AskReplies	operations	124,432.66	122,261.02	120,879.65	1,903.78

Aaronontheweb · 2018-01-12T19:06:40Z

The steep drop in allocations and the improvement in throughput are correlated. Probably GC behavior.

maxcherednik · 2018-01-13T21:23:52Z

@Aaronontheweb Fixed the header and put a comment in the empty method.
Re: perf - so it got better, right? There used to be the more allocations inside that CastTask method. So not anymore.

maxcherednik · 2018-01-26T21:44:31Z

@Aaronontheweb up:)

Aaronontheweb · 2018-01-30T12:55:13Z

Re-running the failed specs...

* Tests should be precise - in temrs of what to expect * Ask interface refined #3220 * ClusterRouter unit test fix #3220 * Ask deadlock test added #3220 * Handle deadlock by removing the SynchronizationContext #3220 * Fixing ScatterGather router test #3220 * Ask interface refined #3220 AskSpecs consolidated Api change approval - internal CastTask removed * Fixing header #3220

maxcherednik force-pushed the askFix branch from c31b446 to 494909f Compare December 21, 2017 08:34

maxcherednik commented Dec 21, 2017

View reviewed changes

maxcherednik mentioned this pull request Dec 27, 2017

Throw on generic Ask if response was Status.Failure #3227

Closed

maxcherednik added a commit to maxcherednik/akka.net that referenced this pull request Dec 27, 2017

Ask interface refined akkadotnet#3220

169b1cf

maxcherednik commented Dec 27, 2017

View reviewed changes

maxcherednik added a commit to maxcherednik/akka.net that referenced this pull request Dec 27, 2017

ClusterRouter unit test fix akkadotnet#3220

0abbff8

maxcherednik force-pushed the askFix branch from 51ca933 to 152d0c1 Compare January 2, 2018 19:10

maxcherednik added 7 commits January 9, 2018 22:11

Tests should be precise - in temrs of what to expect

d34c3dd

Ask interface refined akkadotnet#3220

5d7e0bb

ClusterRouter unit test fix akkadotnet#3220

bf21760

Ask deadlock test added akkadotnet#3220

5bbd12e

Handle deadlock by removing the SynchronizationContext akkadotnet#3220

0b57e06

Fixing ScatterGather router test akkadotnet#3220

054948a

Ask interface refined akkadotnet#3220

bae4ae4

AskSpecs consolidated Api change approval - internal CastTask removed

maxcherednik force-pushed the askFix branch from 95c46c0 to bae4ae4 Compare January 9, 2018 21:16

Merge branch 'dev' into askFix

398c537

Aaronontheweb mentioned this pull request Jan 12, 2018

FAKE discontinues execution of subsequent performance test libraries upon encountering exception #3268

Closed

Aaronontheweb requested changes Jan 12, 2018

View reviewed changes

Fixing header akkadotnet#3220

243131a

Aaronontheweb approved these changes Jan 17, 2018

View reviewed changes

Aaronontheweb added 2 commits January 17, 2018 13:05

Merge branch 'dev' into askFix

48663d9

Merge branch 'dev' into askFix

86078d7

Merge branch 'dev' into askFix

97b997e

Aaronontheweb merged commit d233657 into akkadotnet:dev Jan 30, 2018

Aaronontheweb added this to the 1.3.4 milestone Feb 1, 2018

This was referenced Feb 1, 2018

Akka.NET v1.3.4 release notes #3307

Merged

Akka.NET v1.3.4 Production Release #3309

Merged

maxcherednik deleted the askFix branch February 2, 2018 08:27

		@@ -76,30 +76,29 @@ public class FutureActorRef : MinimalActorRef
		/// TBD

		@@ -0,0 +1,72 @@
		//-----------------------------------------------------------------------
		// <copyright file="ArrayExtensions.cs" company="Akka.NET Project">

Ask interface should be clean #3220

Ask interface should be clean #3220

Conversation

maxcherednik commented Dec 21, 2017 • edited Loading

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

Horusiath commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxcherednik commented Dec 21, 2017

Horusiath commented Dec 21, 2017

maxcherednik commented Dec 21, 2017

Aaronontheweb commented Dec 21, 2017

maxcherednik commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxcherednik commented Dec 21, 2017

Aaronontheweb commented Dec 21, 2017

Aaronontheweb commented Dec 21, 2017

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

maxcherednik commented Dec 27, 2017 • edited Loading

Aaronontheweb commented Dec 27, 2017

maxcherednik commented Dec 27, 2017

Aaronontheweb commented Dec 27, 2017

maxcherednik commented Dec 27, 2017 • edited Loading

to11mtm commented Jan 1, 2018

maxcherednik commented Jan 1, 2018

maxcherednik commented Jan 12, 2018

Aaronontheweb commented Jan 12, 2018

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

Aaronontheweb commented Jan 12, 2018

Before

Totals

Per-second Totals

After

Totals

Per-second Totals

Aaronontheweb commented Jan 12, 2018

maxcherednik commented Jan 13, 2018

maxcherednik commented Jan 26, 2018

Aaronontheweb commented Jan 30, 2018

maxcherednik commented Dec 21, 2017 •

edited

Loading

maxcherednik commented Dec 27, 2017 •

edited

Loading

maxcherednik commented Dec 27, 2017 •

edited

Loading