Clear current message - fixes #1609 #1613

rogeralsing · 2016-01-05T12:23:00Z

cc @JeffCyr please review that the current message on async ActorTaskSchedulerMessage messages looks OK.

rogeralsing · 2016-01-05T12:23:25Z

src/core/Akka/Actor/ActorCell.DefaultMessages.cs

@@ -60,6 +60,7 @@ public void Invoke(Envelope envelope)
                    AutoReceiveMessage(envelope);
                else
                    ReceiveMessage(message);
+                CurrentMessage = null;
            }


From the Scala code

final def invoke(messageHandle: Envelope): Unit = try { currentMessage = messageHandle cancelReceiveTimeout() // FIXME: leave this here??? messageHandle.message match { case msg: AutoReceivedMessage ⇒ autoReceiveMessage(messageHandle) case msg ⇒ receiveMessage(msg) } ///########## current message is cleared here currentMessage = null // reset current message after successful invocation } catch handleNonFatalOrInterruptedException { e ⇒ handleInvokeFailure(Nil, e) } finally { checkReceiveTimeout // Reschedule receive timeout }

rogeralsing · 2016-01-05T14:11:57Z

Some notes.
The rationale for the implementation here is that we should stick to how Scala does this, and all the magic of async operations is dealt with externally.
We set and clear the current message on entry and exit of the ActorCell.Invoke, just like Scala.

When doing this, we also need to make sure that async operations have access to the current message in their continuations, this is solved by capturing the current user message when an async operation starts, and then pass that message back to the actor cell using the ActorTaskSchedulerMessage message.

JeffCyr · 2016-01-05T14:23:29Z

ActorCell.CurrentMessage won't be set after the first await in the async receive. It will be restored properly before calling HandleInvokeFailure in case of an exception, but it won't be set for other usage:

AbstractStash.Stash
DefaultResizer.Pressure
PersistentView.ReplayStarted

Stash won't work after the first await, not sure how it impacts the other usages.

This could be fixed by creating an ActorTaskScheduler instance per async receive, the current message would be kept in the instance so it can be restored on each ActorTaskSchedulerMessage handling.

An instance per message is not that bad for async receive, the throughput of async receive should not be high anyway. However this issue may impact the performance of this approach.

An alternative would be to use a custom ActorSynchronizationContext instead of an ActorTaskScheduler.

rogeralsing · 2016-01-05T14:26:19Z

ActorCell.CurrentMessage won't be set after the first await in the async receive.

I have updated the code to restore the current message on all ActorTaskSchedulerMessage messages.

Do note that there should never be any new or changed current messages while doing an async operation as the mailbox is suspended for user messages.
So IMO, we should be able to just capture the current user message when an async op starts. as it cannot change during that async flow.

JeffCyr · 2016-01-05T14:29:01Z

~~Looks good!~~

JeffCyr · 2016-01-05T14:32:04Z

src/core/Akka/Dispatch/ActorTaskScheduler.cs

-        {
-            _actorCell.Self.Tell(new ActorTaskSchedulerMessage(this, task), ActorRefs.NoSender);
+        {            
+            _actorCell.Self.Tell(new ActorTaskSchedulerMessage(this, task, _actorCell.CurrentMessage), ActorRefs.NoSender);


Hmm wait, I think this approach doesn't work if the task is scheduled from outside of the actor's context.

AFAIK, no one could ever use this outside of the actor context as each scheduler is bound to an individual actor and only used from within a context.
Are there any cases I am missing?
(I can not find any usage in the references that hints that would be the case)

Receive(async m => { // ... await Task.Delay(1000); // ActorTaskScheduler.QueueTask will be called outside the actor's context when the timer expires // ... });

JeffCyr · 2016-01-05T14:41:55Z

@rogeralsing Just saw your edit, that's right ActorTaskScheduler could capture CurrentMessage in a field as long as it is cleared when the async receive completes to not recreate the original issue.

rogeralsing · 2016-01-05T16:17:33Z

src/core/Akka/Dispatch/ActorTaskScheduler.cs

                                  }
-
+                                  //clear the current message field of the scheduler
+                                  actorScheduler.CurrentMessage = null;


Clear the current message of the scheduler

rogeralsing · 2016-01-05T16:19:11Z

cc @JeffCyr , I've updated the scheduler to keep a field for the current message.
I've also updated the tests to ensure that we do have the correct current message after awaiting a Task.Delay.

JeffCyr · 2016-01-05T17:34:42Z

Looks good! 👍

Aaronontheweb · 2016-01-05T18:54:30Z

Should we add a spec to ensure that the current message is cleared?

rogeralsing · 2016-01-06T09:48:14Z

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

+            var actor = Sys.ActorOf(Props.Create(() => new DummyActor(autoResetEvent)));
+            actor.Tell("hello");
+            //ensure the message was received
+            autoResetEvent.WaitOne();


wait until the actor have received our message

rogeralsing · 2016-01-06T09:49:47Z

@Aaronontheweb I've added a spec to ensure that the current message is cleared.

re-set curret message on async callbacks store current message in a field clear CurrentMessage Added clear message spec added async await test

rogeralsing · 2016-01-06T09:59:38Z

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

+            //wait while current message is not null (that is, receive is not yet completed/exited)
+            SpinWait.SpinUntil(() => cell.CurrentMessage == null, TimeSpan.FromSeconds(2));
+
+            cell.CurrentMessage.ShouldBe(null);


wait for current message to become null, or timeout
Then assert that current message is null

You can use AwaitAssert for this in the future, but this change is fine

Aaronontheweb · 2016-01-06T20:48:49Z

Looks like #1608 dirtied the working directory and failed this spec, which is bad. This is an issue with how I've configured TeamCity, not anything to do with our code. I'll get to work on fixing this.

Aaronontheweb · 2016-01-06T21:55:51Z

All of the tests pass - will review this when I get back from a meeting

Aaronontheweb · 2016-01-07T19:23:18Z

Looks fine to me.

…receive-fixes1609 Clear current message - fixes #1609

skotzko added the needs review label Jan 5, 2016

rogeralsing reviewed Jan 5, 2016
View reviewed changes

JeffCyr reviewed Jan 5, 2016
View reviewed changes

rogeralsing reviewed Jan 5, 2016
View reviewed changes

rogeralsing reviewed Jan 6, 2016
View reviewed changes

fixes 1609

6400e86

re-set curret message on async callbacks store current message in a field clear CurrentMessage Added clear message spec added async await test

rogeralsing reviewed Jan 6, 2016
View reviewed changes

Aaronontheweb mentioned this pull request Jan 6, 2016

TeamCity is not cleaning out build artifacts in the working directory #1622

Closed

Aaronontheweb added a commit that referenced this pull request Jan 7, 2016

Merge pull request #1613 from rogeralsing/clear-currentmessage-after-…

aed87e3

…receive-fixes1609 Clear current message - fixes #1609

Aaronontheweb merged commit aed87e3 into akkadotnet:dev Jan 7, 2016

Aaronontheweb removed the needs review label Jan 7, 2016

This was referenced Jan 18, 2016

Akka.NET v1.0.6 release (DEV) #1653

Merged

Akka.NET v1.0.6 release (PRODUCTION) #1654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear current message - fixes #1609 #1613

Clear current message - fixes #1609 #1613

rogeralsing commented Jan 5, 2016

rogeralsing Jan 5, 2016

rogeralsing commented Jan 5, 2016

JeffCyr commented Jan 5, 2016

rogeralsing commented Jan 5, 2016

JeffCyr commented Jan 5, 2016

JeffCyr Jan 5, 2016

rogeralsing Jan 5, 2016

JeffCyr Jan 5, 2016

JeffCyr commented Jan 5, 2016

rogeralsing Jan 5, 2016

rogeralsing commented Jan 5, 2016

JeffCyr commented Jan 5, 2016

Aaronontheweb commented Jan 5, 2016

rogeralsing Jan 6, 2016

rogeralsing commented Jan 6, 2016

rogeralsing Jan 6, 2016

Aaronontheweb Jan 7, 2016

Aaronontheweb commented Jan 6, 2016

Aaronontheweb commented Jan 6, 2016

Aaronontheweb commented Jan 7, 2016

Clear current message - fixes #1609 #1613

Clear current message - fixes #1609 #1613

Conversation

rogeralsing commented Jan 5, 2016

Choose a reason for hiding this comment

rogeralsing commented Jan 5, 2016

JeffCyr commented Jan 5, 2016

rogeralsing commented Jan 5, 2016

JeffCyr commented Jan 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffCyr commented Jan 5, 2016

Choose a reason for hiding this comment

rogeralsing commented Jan 5, 2016

JeffCyr commented Jan 5, 2016

Aaronontheweb commented Jan 5, 2016

Choose a reason for hiding this comment

rogeralsing commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb commented Jan 6, 2016

Aaronontheweb commented Jan 6, 2016

Aaronontheweb commented Jan 7, 2016