Bug: DefaultMailbox.PostSystemMessage CancellationTokenDisposed exception

[woksin]

I see that sometimes when running tests (for instance here https://github.com/asynkron/protoactor-dotnet/actions/runs/4042463067/jobs/6950198101#step:6:111 )
that the DefaultMailbox.PostSystemMessage can throw an exception when the _invoker.CancellationTokenSource is disposed.

This leads ne to the questions:

1. Why is this CancellationTokenSourceeven exposes as an API? (I don't know if there are any reason to why, if so then it would be nice to know 😄 )
2. Why doesn't the IMessageInvoker itself take the responsibility of cancelling its CancellationTokenSource as part of the Disposing it will eventually do whenever the ActorContext FinalizeStopAsync is inevitably called?

And please excuse my ignorance. I'm fairly new to this project and especially the code base 😆

[rogeralsing]

Hi Sindre, good questions.
These are basically parts of the old dungeons of Proto.Actor so I have to refresh my memory on these parts.

From the mailbox code, we have this:

    public void PostSystemMessage(object msg)
    {
        _systemMessages.Push(msg);

        if (msg is Stop)
        {
            _invoker?.CancellationTokenSource?.Cancel();
        }

My guess w/o digging any deeper would be that the same actor has already received a stop message. and thus the cts is disposed.

Why is this not cancelled inside the actor context?
Let's say an actor is currently working on a long running task, it cannot consume any system messages during this time until the user message is done.
cancelling outside of the actor would allow the actor to stop the current task, and then properly consume the stop message.

But I need to have a better look at all this.

[woksin]

My guess w/o digging any deeper would be that the same actor has already received a stop message. and thus the cts is disposed.

Yes that's my guess as well.

Let's say an actor is currently working on a long running task, it cannot consume any system messages during this time until the user message is done.
cancelling outside of the actor would allow the actor to stop the current task, and then properly consume the stop message.

I see... That make sense in terms of what the system should do, thanks for the explanation.

From just my outsider perspective I think that the CancellationTokenSource is perhaps too low-level of an abstraction to fit on IMessageInvoker interface, and frankly what it actually does is hidden in its implementation so it's hard to tell 😄. ; My suggestion, although maybe naive, would be to eventually remove that property (could make it obsolete for now) and replace it with a method for specifically terminating the the current running task.

I also don't have too much experience using Proto.Actor extensively, but since the CancellationTokenSource is on the IMessageInvoker and ActorContext is implementing IMessageInvoker is it intended for the user of the framework to be able to interact directly with this CancellationTokenSource? To me it seems like a way to easily shoot yourself in the foot in terms of havoc you can cause? 😆

[DenizPiri]

We experience the same issue. We have 600+ tests, each starting and shutting down a cluster; usually, 1 or 2 tests at random experience this error.

I believe this is because of the ActorContextExtras.Dispose() function doing a CancellationTokenSource.Dispose() call. CancellationTokenSource doesn't require Dispose() call when only Cancel() is used.

I will submit a PR.

[woksin]

Should we consider this issue solved now with #1920 ?