8

I'm wrapping AspNet.Identity. But something confuses me about TPL.

First example:

    public virtual async Task<IdentityResult> RemovePasswordAsync(string userId)
    {
        var user = _store.FindByIdAsync(userId).Result;
        if (user == null)
            throw new InstanceNotFoundException("user");

        user.PasswordHash = String.Empty;
        user.SecurityStamp = String.Empty;
        return await UpdateAsync(user);
    }

    public virtual async Task<IdentityResult> UpdateAsync(TUser user)
    {
        await _store.UpdateAsync(user);
        return new IdentityResult();
    }

Second example:

    public virtual Task<IdentityResult> RemovePasswordAsync(string userId)
    {
        var user = _store.FindByIdAsync(userId).Result;
        if (user == null)
            throw new InstanceNotFoundException("user");

        user.PasswordHash = String.Empty;
        user.SecurityStamp = String.Empty;
        return UpdateAsync(user);
    }

    public virtual async Task<IdentityResult> UpdateAsync(TUser user)
    {
        await _store.UpdateAsync(user);
        return new IdentityResult();
    }

And client will call this like:

    result = await _userManager.RemovePasswordAsync(user.Id);

My first question is:

When the client calls the second method, the work is offloaded to a threadpool thread from the IO thread. And when RemovePasswordAsync is called it calls UpdateAsync which has an await keyword. So, at that point does this threadpool thread offload to another threadpool thread? Or does TPL continue using the same thread instead?

And my second question is; what is the main difference between the first implementation and the second implementation of constructing this async method?

EDIT:

This is the update method of the UserStore class. (_store.UpdateAsync(user))

    public Task UpdateAsync(TUser user)
    {
        if (user == null)
            throw new ArgumentNullException("user");

        return _userService.UpdateAsync(user);
    }

And this is the update method of the UserService class

    public Task UpdateAsync(TUser user)
    {
        return Task.Factory.StartNew(() => Update(user));
    }
Kristof U.
  • 1,263
  • 10
  • 17
  • 2
    The second question: http://stackoverflow.com/q/21033150/1768303 – noseratio Feb 25 '14 at 13:27
  • 3
    You actually shouldn't use StartNew (in web applications) - The reason being is that when you await, the executing thread gets sent back to the CLR threadpool, when you start new you pull a thread from the CLR threadpool. This is counter-productive in web applications. (you'll get thread starvation, IIS won't be able to assign new requests to a thread, so your HTTP.Sys (http stack) input queue will fill up and your users will start getting denied) – Steve's a D Feb 25 '14 at 13:45
  • @Steve Thread starvation? How? You're freeing the request thread (so that asp.net can use it to server other incoming requests) and offloading to a threadpool thread. – dcastro Feb 25 '14 at 13:57
  • @dcastro They're using the same threadpool. IIS receives a request, pulls a thread from the CLR threadpool.... startnew pulls a thread from the CLR threadpool. What should happen is the thread should be returned to the CLR threadpool, and then the OS should interrupt the CLR to get a new thread whenever its task is complete. StartNew doesn't do that. – Steve's a D Feb 25 '14 at 13:58
  • Just to be more clear, you'll get thread starvation because your long running task is still using the CLR threadpool. – Steve's a D Feb 25 '14 at 14:03
  • 2
    @Steve You're right, I was under the impression that ASP.NET used its own set of threads, not the ThreadPool. Knowing this, it makes perfect sense to avoid switching threads like this, all you'd be doing is adding overhead. For whoever's interested, Stephen Cleary's [Task.Run Etiquette Examples: Don't Use Task.Run in the Implementation](http://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html) – dcastro Feb 25 '14 at 14:08
  • @Steve So what I infer is this; if its not cpu-bound work, and if its a web app (exactly my case..) you should not use `Factory.StartNew`, also I know that `Task.Run` should be used when the work is cpu-bound, so is my example is the right place to use `Task.FromResult` ?? – Kemâl Gültekin Feb 25 '14 at 14:29
  • @KemalGültekin I think the idea is, don't use tasks or async/await at all, make it all run synchronously. I can't think of many situations where async/await would be useful in an ASP.NET scenario, except for I/O-bound work (where you'd be using an I/O completion port, and *not* a ThreadPool thread). – dcastro Feb 25 '14 at 14:33
  • 1
    Also, this: http://stackoverflow.com/a/2642789/857807 – dcastro Feb 25 '14 at 15:44

2 Answers2

3

I'll answer your first question.

You're misunderstanding how async/await works.

An async method will run synchronously at least until it hits the first await statement. When it hits an await, it has two options:

  • If the awaitable (e.g. Task) has already completed, execution carries on the current context (i.e. UI Thread, or ASP.NET request's context).
  • If the awaitable hasn't completed yet, it wraps the rest of the method's body and schedules that to be executed on the current context (i.e. UI Thread) (*) when the task completes.

By this definition, your whole code will run on the same ASP.NET request's context.

_store.UpdateAsync may however spawn a ThreadPool thread (e.g., by using Task.Run).

Updated

According to your updated answer, Update(user) will run on a ThreadPool thread. Everything else will run on the current context.


(*) The rest of the method's body will be scheduled to run on a ThreadPool thread if there's no synchronization context (i.e., Console Application).

dcastro
  • 66,540
  • 21
  • 145
  • 155
  • Yes I think you are right, _store.UpdateAsync causes it to offload to another thread. But think of a method in which at the beginning I simply invoke that async method like this: `var removePasswordTask = RemovePasswordAsync(user.Id); //...... //...... var result = await removePasswordTask;` Since this task is hot, it will be running the moment I delegate it to a task variable. So from the moment I delegate it until the moment I await it, where does the execution takes place? – Kemâl Gültekin Feb 25 '14 at 13:44
  • 1
    @KemalGültekin Take a look at Gotcha #1 here: [Async in C# and F#: Asynchronous gotchas in C#](http://tomasp.net/blog/csharp-async-gotchas.aspx/) – dcastro Feb 25 '14 at 13:50
  • 1
    @KemalGültekin Even if you don't await the task returned by `RemovePasswordAsync`, the call will run synchronously until it hits `Task.Factory.StartNew`. Then, `Update(user)` is offloaded to the ThreadPool and a `Task` representing this job is returned. This task is bubbled up until it hits the top level method, which then executes `//...//...`. Then, the top-level method `awaits` this task - the rest of your method (i.e., whatever comes after the `await`) will be scheduled to run on the current context later. – dcastro Feb 25 '14 at 13:54
  • 2
    @KemalGültekin Note that if you did this, you'd be achieve parallelism. You'd be executing `Update(user)` and `//...` concurrently. However, you'd be "trading scalability (how many clients can you can service) for performance (how quickly you can return the response to a single client)" (quoted from [Brad Wilson](http://www.hanselman.com/blog/TheMagicOfUsingAsynchronousMethodsInASPNET45PlusAnImportantGotcha.aspx)). – dcastro Feb 25 '14 at 14:53
2

And my second question is; what is the main difference between the first implementation and the second implementation of constructing this async method?

Your first implementation can and should be improved by replacing the blocking _store.FindByIdAsync(userId).Result with asynchronous await _store.FindByIdAsync(userId):

public virtual async Task<IdentityResult> RemovePasswordAsync(string userId)
{
    var user = await _store.FindByIdAsync(userId);
    if (user == null)
        throw new InstanceNotFoundException("user");

    user.PasswordHash = String.Empty;
    user.SecurityStamp = String.Empty;
    return await UpdateAsync(user);
}

Without such update, the difference is perhaps described best by Eric Lippert here. One particular thing is how exceptions can possibly be thrown and handled.

Updated to address the comments. You should not be offloading with Task.Factory.StartNew or Task.Run in ASP.NET. This is not a UI app where you need to keep the UI responsive. All this does is just adding an overhead of a thread switch. The HTTP request handler which calls Task.Run then awaits or blocks will take at least the same number of threads to complete, you will not improve scalability and only hurt the performance. On the other hand, it does make sense to use naturally IO-bound tasks, which don't use a thread while pending.

Community
  • 1
  • 1
noseratio
  • 59,932
  • 34
  • 208
  • 486
  • You are right, without that await, the method should not be marked as async because of the overload.. But I have another question; when I await that task it continues to run synchroniously until a Task.Run or Factory.StartNew etc.. So until than its using the same thread, but at that point its offloading to another thread until the await statement, which at that point is returning to calling thread. So at this particular example; isn't it better to remove the async keyword and call FindByIdAsync.Result hence blocking and at the end returning the task by using Task.FromResult – Kemâl Gültekin Feb 25 '14 at 14:50
  • @KemalGültekin In that case, `.Result` wouldn't block, because the result would be immediately available, since the whole thing ran synchronously. So you might as well remove tasks altogether. – dcastro Feb 25 '14 at 14:59
  • @dcastro Yep you are right but, I still have to use tasks to satisfy the interface though. – Kemâl Gültekin Feb 25 '14 at 15:11
  • @KemalGültekin Can you not change the interface? Are you being forced to do this? – dcastro Feb 25 '14 at 15:12
  • @dcastro I have extracted interface from Microsoft's implementation of UserManager and I want my library to be consistent with it. I may create overloads but then it causes the client to confuse (async or sync?). Also interfaces of aspnet identity forces me to implement async methods, and its not good to consume sync stuff in an async method. So if i'm to change that to sync, I have to implement sync overloads of each method in each identity interface. Actually I think we are back to what you said, async await should be avoided completely in such cases. – Kemâl Gültekin Feb 25 '14 at 15:24
  • 1
    @KemalGültekin You should avoid this, but if you can't change the interface, then return `Task.FromResult(Update(user))`. But DON'T use `.Result`. Although in this scenario it's perfectly fine to block on your "synchronous task", if the implementation ever changes and decides to use `Task.Run` instead of `Task.FromResult`, clients may run into a deadlock (a common problem described [here](http://blog.stephencleary.com/2012/07/dont-block-on-async-code.html)). So, in conclusion, I would use `Task.FromResult` and `await` that task at the top-level. – dcastro Feb 25 '14 at 15:27