High volume POST controller asp core 1.1

Question

I need to have a POST controller handling a lot of traffic I've done something like that

    [HttpPost]
    public async Task<IActionResult> Post([FromBody] List<VdoPlayInfo> lv)
    {
        var seesionID = HttpContext.Session.GetString("sessionId");
        var vdoID = HttpContext.Session.GetInt32("videoid");
        var tasks = new Task[lv.Count];
        for (int i = 0; i < lv.Count; i++)
        {
            tasks[i] = Task.Run(() => _ADB.SaveLogView(lv[i]);                    
        }
        await Task.WhenAll(tasks);
        return Ok();          
    }

after several seconds of working I get this error

System.ArgumentOutOfRangeException: 'Index was out of range. 
Must be non-negative and less than the size of the collection.'

even tough the lv.Count = 2 I get the i index reaching 5. Whats the problem?

See [here](https://stackoverflow.com/questions/1757112/anonymous-c-sharp-delegate-within-a-loop) — ProgrammingLlama, Nov 05 '17 at 13:13
@Kulpemovitz are you sure that ` _ADB.SaveLogView(lv[i])` doesn't do anything to your list? — BRAHIM Kamel, Nov 05 '17 at 13:14
BRAHIM Kamel - doing nothing. I just push it to the DB using Dapper and SP — Kulpemovitz, Nov 05 '17 at 13:19
John - 10x for the tip. I know this issue. it didnt help me. for some reason all the thread are accessing the same "i" var inside the loop — Kulpemovitz, Nov 05 '17 at 13:21
try this: var tasks = lv.Select(o => Task.Run(() => _ADB.SaveLogView(o)), and then await Task.WhenAll(tasks) — barakcaf, Nov 05 '17 at 13:51
Don't start threads in ASP.NET / ASP.NET Core applications, it will mess up with the thread pool. Make your `SaveLogView` method async and call Dappers async methods instead. — Tseng, Nov 05 '17 at 21:41
@Kulpemovitz, in order for commenters to notice you've answered them, prefix their name with '@' sign. this will alert them of your answer, and show a tooltip above the text box of the alias you should use (usually the name without spaces). — Tsahi Asher, Nov 07 '17 at 15:20

score 0 · Answer 1 · answered Nov 05 '17 at 13:40

0

Could you try this?

for (int i = 0; i < lv.Count; i++)
    {
        var item = lv[i];
        tasks[i] = Task.Run(() => _ADB.SaveLogView(item);                    
    }

answered Nov 05 '17 at 13:40

valcarcel

1
1

score 0 · Accepted Answer · answered Nov 06 '17 at 04:59

It's not clear what you're ultimately trying to do here, but if your goal is high performance, this is the absolute wrong way to achieve that. Task.Run is going to pull a new thread from the pool each time you call it, so with two items in your list, your action is now consuming three threads (one for the request and one for each list item). Obviously, as the list items scale, so does your thread usage, so you're dramatically cutting the potential throughput of your server, and may actually end up being thread-starved.

It's not clear what _ADB.SaveLogView is doing, but if it does synchronous work, you need to realize that using Task.Run doesn't make it async. Sync is still sync, you're just blocking a different thread instead of the request thread. However, since you're awaiting task completion, the request thread is still blocked, so you're actually not achieving anything other than just wasting a bunch of threads that could be handling other requests.

If _ADB.SaveLogView is async (in which case you should name it _ADB.SaveLogViewAsync to avoid confusion), then you can simplify your code (and avoid wasting threads) by just doing:

var tasks = new Task[lv.Count];
for (int i = 0; i < lv.Count; i++)
{
    tasks[i] = _ADB.SaveLogViewAsync(lv[i]);                    
}

Since it should already be returning a Task in that scenario, there's no need to wrap it in another Task.

However, ideally, you should actually being handling this transactionally. If there's a problem saving one of these entities, the others still get saved. That can cause problems then if you need to resubmit to save the entity or entities that failed. Again, it's not clear what this method is doing, but with something like Entity Framework, you would simply add each new entity to the DbSet (which marks them as to be created in the change tracking) and then call SaveChangesAsync just once, which would attempt to save all the items in one go, wrapped in a transaction, so if any fail, everything gets rolled back.

If you're using Entity Framework or some other system that supports transactions under the hood, you should be utilizing that functionality. If you're doing some sort of manual database work, then you should roll your own transactions.

Thank you Chris - great stuff. "SaveLogView" is a simple function to store the data into the SQL server using Dapper and Stored Procedure. for making the store procedure async is just adding async in the function line? — Kulpemovitz, Nov 06 '17 at 09:56
No. You make it async by calling async methods. Dapper supports async usage, so you just need to utilize it that way. — Chris Pratt, Nov 06 '17 at 12:28

High volume POST controller asp core 1.1

2 Answers2