0

I'm not sure if this code is Asynchronous. I call this function using await from my main controller and within the function I use await on the LINQ query and .ToListAsync() - but after the query I have the foreach loop which may defeat the purpose of async on the query.

Main Controller Call:

case "getassets":
    reply = await GetAssets();
    break;

Function:

public async Task<ReplyObj> GetAssets()
{
    ReplyObj obj = new ReplyObj();
    obj.Result = new List<dynamic>();
    dynamic AssetRecords = await _context.Asset.FromSql("SELECT * FROM Asset").ToListAsync();

    foreach (var objAsset in AssetRecords)
    {
        obj.Result.Add(new Asset() 
        {
            AssetId = objAsset.AssetId,
            Name = objAsset.Name,
            Description = objAsset.Description,
            PriceDecimals = objAsset.PriceDecimals 
        });
    }

    obj.Success = true;
    obj.Message = "";

    return obj;        
}

This call will have many requests hitting it, I want to know for sure that its using async correctly. Thank you!

maccettura
  • 10,514
  • 3
  • 28
  • 35
Nick D
  • 81
  • 1
  • 5
  • Why do you think the loop would defeat the purpose? The DB call is going to take much much longer than that loop and during that time the thread is free to do other stuff. – juharr Aug 14 '19 at 19:14
  • Why isn't the `ReplyObj.Result` a dynamic? What about a `ReplyObj`? – Jeroen van Langen Aug 14 '19 at 19:17
  • The only async on this is `ToListAsync`. until that will be ran on the current thread. You should run the query using `Task.Run()` in combination with `TaskCompletionSource`. Probably the are already FromSqlAsync methods or extensions – Jeroen van Langen Aug 14 '19 at 19:19
  • @JeroenvanLangen Saying "until that will be ran on the current thread" makes it sound like awaiting ToListAsync will somehow spin up a different thread, which is not correct – chill94 Aug 14 '19 at 20:35
  • Because I felt like the await should cover the entire function, not just part of it. However that does make sense thank you @juharr - ReplyObj contains a dynamic list "Result". – Nick D Aug 15 '19 at 00:31
  • @chill94 that's a little nitpicking, but _`which is not correct`_ is bluntly. – Jeroen van Langen Aug 15 '19 at 06:49
  • 1
    @JeroenvanLangen Putting something that is primarily IO bound into `Task.Run` is mostly pointless, it's better to use actual async methods for IO and use `Task.Run` for CPU bound (or IO if there are no async methods available). – juharr Aug 15 '19 at 12:47

2 Answers2

1

To begin, here's a couple of references for async/await in C# that I'd suggest reviewing:

The simple (high-level) answer is that awaiting your sql call will return control up the call stack and continue execution. In this case, that means it will go up to:

reply = await GetAssets();

Which will in turn return control to whatever function called that, etc. etc..

With that said, if all of your async calls in your call stack are immediately being awaited, then async won't end up buying you anything/changing the flow of control. To say, keep in mind that async != threading.

chill94
  • 341
  • 1
  • 5
  • This is where I was a bit confused, so its like a cycle between calling the function and the await on the Query, well what happens with the foreach loop, does that start blocking the thread? – Nick D Aug 15 '19 at 00:33
  • Running CPU bound code (ie. processing data in a foreach loop) will not yield control back up the call stack. Does that help? I'd encourage you to keep digging into async/await to make sure you've got a solid grasp of what's going on, as it can definitely be somewhat tricky when you're first getting started. Though I was criticized for pointing it out above, one of the biggest takeaways I can offer is that it awaiting does not create new threads. It creates a state machine on the current thread, giving it a point at which to resume when the IO bound call completes (on the same thread). – chill94 Aug 15 '19 at 15:10
  • I realized after re-reading my post that the parenthetical note, "(on the same thread)", is an incorrect statement. Here's a link pointing out [my error](https://stackoverflow.com/questions/21838651/i-thought-await-continued-on-the-same-thread-as-the-caller-but-it-seems-not-to). – chill94 Aug 15 '19 at 21:33
0

Few things that I want to point out:

  1. dynamic AssetRecords = await _context.Asset.FromSql("SELECT * FROM Asset").ToListAsync();
    When you are using _context.Asset it will return all the Asset rows that you have. Why would you execute another query on the table when the Asset it self is giving all that you need? Hence, to me this is redundant thing.

  2. And if you use select method and then get the list asyncronously then you will have eliminated the the foreach loop and it will cost only one await call and query processing. See code sample below:

    public async Task<ReplyObj> GetAssets()
{
    ReplyObj obj = new ReplyObj();
    return obj.Result = await _context.Asset.Select(s => new
    {
        AssetId = s.AssetId,
        Name = s.Name,
        Description = s.Description,
        PriceDecimals = s.PriceDecimals
    }).ToListAsync();
}

This could now be your true async method.

PS: If you want to show your code to experts for review, I would suggest joining StackExchange CodeReview Community

Jamshaid K.
  • 3,555
  • 1
  • 27
  • 42
  • Thank you, this is exactly what looks and feels right, the await now covers the entire function. Your right it was redundant thank you for pointing that out, I was really just after the Async functionality. – Nick D Aug 15 '19 at 00:30
  • Your code gives me: Cannot implicitly convert type 'System.Collections.Generic.List<>' to 'System.Collections.Generic.List' Is there a way to apply the Asset type to the object I'm creating.. or make these two compatible? – Nick D Aug 15 '19 at 00:52
  • I fixed it, here's the new code: obj.Result = await _context.Asset.Select(s => new Asset { AssetId = s.AssetId, Name = s.Name, Description = s.Description, PriceDecimals = s.PriceDecimals }).Cast().ToListAsync(); – Nick D Aug 15 '19 at 01:12