Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed fix for #886 asp.net deadlock #905

Merged
merged 2 commits into from
Apr 25, 2015

Conversation

kekekeks
Copy link
Contributor

See #886

@rogeralsing
Copy link
Contributor

Looks good to me, anyone verified if this solves the asp.net deadlocks?
The await's are gone from the code, but from now on, I dont trust TPL :suspect:

@rogeralsing rogeralsing changed the title Proposed fix for #886 Proposed fix for #886 asp.net deadlock Apr 25, 2015
@kekekeks
Copy link
Contributor Author

    public class ActorController : Controller
    {
        //
        // GET: /Actor/


        public class MyActor : UntypedActor
        {
            protected override void OnReceive(object message)
            {
                Sender.Tell(message + "-pong");
            }
        }

        public ActionResult Index()
        {
            var actor = MvcApplication.ActorSystem.ActorOf(Props.Create<MyActor>());
            var res = actor.Ask<string>("ping").Result;
            return new ContentResult() {Content = res};
        }

    }

At least this code is working now

@nvivo
Copy link
Contributor

nvivo commented Apr 25, 2015

There are more things happenning here.

  1. The Ask's are in the reverse order. Today, Ask<object> call Ask<T> that calls self.Ask<object> with await. This is all unnecessary. Just by moving all logic to Ask<object> and making Ask<T> just cast the result achieves the same result. The CastTask method is then unnecessary.
  2. I suspect this doesn't actually fix the deadlock. Can you run this with actor selection to an actor running somewhere else?

@kekekeks
Copy link
Contributor Author

    {

        public ActionResult Index()
        {

            var actor = MvcApplication.ActorSystem.ActorSelection("/user/test");
            var res = actor.Ask<string>("ping").Result;
            return new ContentResult() {Content = res};
        }

    }
    public class MvcApplication : System.Web.HttpApplication
    {

        public class MyActor : UntypedActor
        {
            public MyActor()
            {

            }
            protected override void OnReceive(object message)
            {
                Sender.Tell(message + "-pong");
            }
        }

        public static ActorSystem ActorSystem = ActorSystem.Create("sandbox");
        protected void Application_Start()
        {
            ThreadPool.QueueUserWorkItem(_ => MvcApplication.ActorSystem.ActorOf(Props.Create<MyActor>(), "test"));
            AreaRegistration.RegisterAllAreas();

            WebApiConfig.Register(GlobalConfiguration.Configuration);
            FilterConfig.RegisterGlobalFilters(GlobalFilters.Filters);
            RouteConfig.RegisterRoutes(RouteTable.Routes);
            BundleConfig.RegisterBundles(BundleTable.Bundles);
        }
    }

Works in this scenario

@kekekeks
Copy link
Contributor Author

Just by moving all logic to Ask and making Ask just cast the result achieves the same result. The CastTask method is unecessary.

You can't do that. You have to move the cast to FutureActorRef (which is public and is used in a lot of places) and change it from FutureActorRef to FutureActorRef<T>

@nvivo
Copy link
Contributor

nvivo commented Apr 25, 2015

Talking different things here. What I mean is:

public static Task<object> Ask(this ICanTell self, object message, TimeSpan? timeout = null)
{
    return self.Ask<object>(message, timeout);
}

public static async Task<T> Ask<T>(this ICanTell self, object message, TimeSpan? timeout = null)
{
    IActorRefProvider provider = ResolveProvider(self);
    if (provider == null)
        throw new NotSupportedException("Unable to resolve the target Provider");

    ResolveReplyTo();
    var result = (T)await Ask(self, message, provider, timeout);
    return result;
}

First should read as:

public static Task<object> Ask(this ICanTell self, object message, TimeSpan? timeout = null)
{
    IActorRefProvider provider = ResolveProvider(self);
    if (provider == null)
        throw new NotSupportedException("Unable to resolve the target Provider");

    ResolveReplyTo();

    return Ask(self, message, provider, timeout);
}

public static async Task<T> Ask<T>(this ICanTell self, object message, TimeSpan? timeout = null)
{
    return (T) await Ask(self, message, timeout);
}

The internal Ask already returns a Task<object>, there is no need for all the indirection. This doesn't change the problem, just makes the code more readable and in the correct order.

Then you can optimize the await with some ContinueWith logic to execute in the same thread that just casts the value, but that is much more simple than that.

With code in the right order, you could just avoid the entire casting logic inside

var result = (string) actor.Ask("ping").Result;

That's what I mean.

@Aaronontheweb
Copy link
Member

@kekekeks could you please run the test suite with the debugger attached and see what it's failing on Akka.Tests? Normally when this issue comes up it's a StackOverflowException related to XUnit and the TestKit both racing to dispose a resource, but the error message will not show up in the build server.

@kekekeks
Copy link
Contributor Author

Should work now

rogeralsing added a commit that referenced this pull request Apr 25, 2015
@rogeralsing rogeralsing merged commit 9210ebb into akkadotnet:dev Apr 25, 2015
@rogeralsing
Copy link
Contributor

Im pulling this in, tests are passing and the code is cleaner than we have atm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants