0

I would like to know if the code I produced is good practice and does not produce leaks, I have more than 7000 objects Participant which I will push individually and Handle the Response to save the "external" id in our database. First I use the Parallel ForEach on the list pPartcipant:

Parallel.ForEach(pParticipant, participant =>
{
try
{
    //Create
    if (participant.id == null)
    {
        ExecuteRequestCreate(res, participant);
    }
    else
    {//Update
        ExecuteRequestUpdate(res, participant);
    }
}
catch (Exception ex)
{
    LogHelper.Log("Fail Parallel ", ex);
}
});

Then I do a classic (not async request), but after I need to "handle" the response (print in the console, save in a text file in async mode, and Update in my database)

    private async void ExecuteRequestCreate(Uri pRes, ParticipantDo pParticipant)
    {
        try
        {
            var request = SetRequest(pParticipant);

            //lTaskAll.Add(Task.Run(() => { ExecuteAll(request, pRes, pParticipant); }));
            //Task.Run(() => ExecuteAll(request, pRes, pParticipant));
            var result = RestExecute(request, pRes);
            await HandleResult(result, pParticipant);
            //lTaskHandle.Add(Task.Run(() => { HandleResult(result, pParticipant); }));
        }
        catch (Exception e)
        {
            lTaskLog.Add(LogHelper.Log(e.Message + " " + e.InnerException));
        }
    } 

Should I run a new task for handeling the result (as commented) ? Will it improve the performance ? In comment you can see that I created a list of tasks so I can wait all at the end (tasklog is all my task to write in a textfile) :

       int nbtask = lTaskHandle.Count;
        try
        {
            Task.WhenAll(lTaskHandle).Wait();
            Task.WhenAll(lTaskLog).Wait();
        }

        catch (Exception ex)
        {
            LogHelper.Log("Fail on API calls tasks", ex);
        }

I don't have any interface it is a console program.

Benoît
  • 143
  • 1
  • 2
  • 15
  • 2
    1) `Parallel` class was designed for CPU intensive jobs, not for I/O bound parallelism 2) Do not use `async void` prefer `async Task` 3) Do not use `Wait()` prefer `await` – Peter Csala Mar 11 '21 at 14:43
  • @PeterCsala Thanks, but what's inside the loop does it matter that it is an async function ? – Benoît Mar 11 '21 at 14:54
  • I used a Wait() method to purposely wait the end of all tasks and finish properly the program since I do some logs after each call I cannot close the program. I am aware of the difference between wait() and await :) – Benoît Mar 11 '21 at 14:58
  • 1
    `Parallel.Foreach` does NOT have [any overload](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.parallel.foreach) which accepts async method. If you wish to perform I/O operations concurrently then please prefer `await Task.WhenAll` – Peter Csala Mar 11 '21 at 14:59
  • First and foremost, [avoid async void](https://learn.microsoft.com/en-us/archive/msdn-magazine/2013/march/async-await-best-practices-in-asynchronous-programming#avoid-async-void). Currently the `Parallel` class has no support for parallelizing asynchronous workloads. It's possible that a `ForEachAsync` method will be added in the next major .NET release. For now, you could take a look at [multiple custom solutions](https://stackoverflow.com/questions/15136542/parallel-foreach-with-asynchronous-lambda) to this problem. – Theodor Zoulias Mar 11 '21 at 14:59

1 Answers1

2

I would like to know if the code I produced is good practice

No; you should avoid async void and also avoid Parallel for async work.

Here's a similar top-level method that uses asynchronous concurrency instead of Parallel:

var tasks = pParticipant
    .Select(participant =>
    {
      try
      {
        //Create
        if (participant.id == null)
        {
          await ExecuteRequestCreateAsync(res, participant);
        }
        else
        {//Update
          await ExecuteRequestUpdateAsync(res, participant);
        }
      }
      catch (Exception ex)
      {
        LogHelper.Log("Fail Parallel ", ex);
      }
    })
    .ToList();
await Task.WhenAll(tasks);

And your work methods should be async Task instead of async void:

private async Task ExecuteRequestCreateAsync(Uri pRes, ParticipantDo pParticipant)
{
  try
  {
    var request = SetRequest(pParticipant);
    var result = await RestExecuteAsync(request, pRes);
    await HandleResult(result, pParticipant);
  }
  catch (Exception e)
  {
    LogHelper.Log(e.Message + " " + e.InnerException);
  }
} 
Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • Thank you, my question is : Awaiting the restexecute and Handle result will not affect the time execution ? I have many participant, I don't need to wait for one result participant to go to the next ? – Benoît Mar 11 '21 at 15:15
  • 1
    No; `await` doesn't affect time execution. The `WhenAll` pattern runs them all concurrently. – Stephen Cleary Mar 11 '21 at 15:49
  • 1
    Than you :) by the way I get an error the lambda expression should be asyn too : var tasks = pParticipant.Select(async participant =>..... – Benoît Mar 11 '21 at 16:20
  • Why not using a list of task ? List(); – Benoît Mar 11 '21 at 16:36
  • `tasks` *is* a `List`. – Stephen Cleary Mar 11 '21 at 16:38
  • To handle the task result can I put it in a list too ? lTaskHandle.Add(Task.Run(() => { HandleResult(result, pParticipant); })); Or awaiting it won't change anything ? the handle performa writing in text file (async) and update in database – Benoît Mar 11 '21 at 16:56
  • @Benoît: Sorry, I'm not clear on what you're asking. You *can* put tasks in lists, sure, but why do you need to? – Stephen Cleary Mar 11 '21 at 18:23
  • I don't need to, I was wondering how I can hande all the tasks, by putting it in a list istead of awaiting I know whenAll are done. But the first list you created when you await, it will also await all sub task ? – Benoît Mar 12 '21 at 10:52
  • 1
    @Benoît: Yes. Because `ExecuteRequestCreateAsync` uses a normal `await`, and the `Select` delegate uses a normal `await`, then each task in that list will not complete until those subtasks are complete. You may find my [`async` intro](https://blog.stephencleary.com/2012/02/async-and-await.html) helpful. – Stephen Cleary Mar 12 '21 at 13:28