7
\$\begingroup\$

I have a Timer:

var QueryReportTimer = new Timer(QueryReportTimerCallback, null, TimeSpan.Zero, TimeSpan.FromSeconds(15));

The important part here is the method QueryReportTimerCallback.

I have a few doubts here:

  • Is it ok to have an async void method? In general it is not, but how can I avoid this when the timer callback is delegate public delegate void TimerCallback(object state);?
  • Is it ok to have await inside Parallel.ForEach?

Method QueryReportTimerCallback:

private async void QueryReportTimerCallback(object state)
{
    if (await semaphoreQueryReportSlim.WaitAsync(10))
    {
        try
        {
            if (machineConfigurations != null)
            {
                await Task.Factory.StartNew(() =>
                    Parallel.ForEach(machineConfigurations.Where(x => x.QueryReport), async (configuration) =>
                    {
                        if (configuration.IsConnectionValid)
                        {
                            var queryReport = new QueryReport(configuration, ReportConfigurations, fileContainer, applicationConfiguration, logger);
                            await QueryAReport(configuration, queryReport);
                        }
                    })
                );
            }
        }
        catch (Exception e)
        {
            logger.LogError(e, e.Message);
        }
        finally
        {
            semaphoreQueryReportSlim.Release();
        }
    }
}
\$\endgroup\$
0

2 Answers 2

15
\$\begingroup\$

Is it ok to have an async void method?

Referencing Async/Await - Best Practices in Asynchronous Programming

As already stated in the OP async void should be avoided as much as possible.

The one exception to that rule being for event handlers, which can be the loophole to achieving the desired behavior while still having the ability to catch and handle any thrown exceptions.

Create an event

event EventHandler QueryReportCallbackEvent = delegate { };

to be raised by the timer callback

private void QueryReportTimerCallback(object state) {
    QueryReportCallbackEvent(this, EventArgs.Empty);
}

The event handler allows async void, so now you can safely do

private async void QueryReportCallbackEventHandler(object sender, EventArgs e) {
    if (await semaphoreQueryReportSlim.WaitAsync(10))
        await queryReportsCore();
}

Is it ok to have await inside Parallel.ForEach?

NO!!!!

The async lambda

async (configuration) => ...

will be converted to async void which takes us right back to what was said in the beginning. async void BAD!!!

Instead refactor that lambda out into its own method that returns a Task

private async Task HandleReport(MachineConfiguration configuration) {
    if (configuration.IsConnectionValid) {
        var queryReport = new QueryReport(configuration, ReportConfigurations, fileContainer, applicationConfiguration, logger);
        await QueryAReport(configuration, queryReport);
    }
}

and this will now allow for the use of Task.WhenAll with all the machine configurations retrieved from the query.

var tasks = machineConfigurations
                .Where(x => x.QueryReport)
                .Select(configuration => HandleReport(configuration));

await Task.WhenAll(tasks);

This actually removes the need for the Paralell.ForEach.

Here is the complete code for what was described above.

//CTOR
public MyClass() {
    QueryReportCallbackEvent += QueryReportCallbackEventHandler;
    var QueryReportTimer = new Timer(QueryReportTimerCallback, null, TimeSpan.Zero, TimeSpan.FromSeconds(15));
}

event EventHandler QueryReportCallbackEvent = delegate { };

private void QueryReportTimerCallback(object state) {
    QueryReportCallbackEvent(this, EventArgs.Empty);
}

private async void QueryReportCallbackEventHandler(object sender, EventArgs e) {
    if (await semaphoreQueryReportSlim.WaitAsync(10))
        await queryReportsCore();
}

private async Task queryReportsCore() {
    try {
        if (machineConfigurations != null) {
            var tasks = machineConfigurations
                            .Where(x => x.QueryReport)
                            .Select(configuration => HandleReport(configuration));

            await Task.WhenAll(tasks);
        }
    } catch (Exception e) {
        logger.LogError(e, e.Message);
    } finally {
        semaphoreQueryReportSlim.Release();
    }
}

private async Task HandleReport(MachineConfiguration configuration) {
    if (configuration.IsConnectionValid) {
        var queryReport = new QueryReport(configuration, ReportConfigurations, fileContainer, applicationConfiguration, logger);
        await QueryAReport(configuration, queryReport);
    }
}

Lastly take note of how the functions were broken down into smaller chunks that allowed for cleaner, more easy to read code.

\$\endgroup\$
10
  • \$\begingroup\$ Thank you for your answer. In my case I swallow an exceptions. Does this mean that I don't need to worry about "async void" in callback method? If we ignore fact that would be nicer, cleaner, more readable and by the book. \$\endgroup\$ Feb 12, 2018 at 8:41
  • \$\begingroup\$ @Raskolnikov Where are you swallowing exception? if an exception is thrown in your async ForEach callback it wont get caught by that catch block you have there as it will be on a different thread. async voids are also called fire and forget as there is no telling what happens to them when invoked. \$\endgroup\$
    – Nkosi
    Feb 12, 2018 at 11:57
  • \$\begingroup\$ I am trying to understand what is benefit of using EventHandler? In both cases you will lose information about exception. Since event handler is also async void. \$\endgroup\$ Feb 12, 2018 at 12:08
  • 3
    \$\begingroup\$ But... Parallel.ForEach runs something in parallel, while WhenAll guarantees that all tasks will be finished by this point of the execution, but DOES NOT guarantees parallelism, both things are way different, am I mistaken? Why would exchange one with another? \$\endgroup\$
    – kuskmen
    Feb 12, 2018 at 14:50
  • 2
    \$\begingroup\$ Task.WhenAll is not always, and in my case ever, a suitable replacement for Parallel.ForEach because I use the overload which limits how many concurrent processes can be run. I've tried swapping this out with the SemaphoreSlim pattern, but performance-wise, it has been so much faster for me to use Parallel.ForEach and just do a .Wait() if I have asynchronous processing to deal with. \$\endgroup\$
    – Dinerdo
    Aug 5, 2019 at 16:06
2
\$\begingroup\$

.net6 now supports Parallel.ForEachAsync

https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.parallel.foreachasync?view=net-6.0

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to the Code Review Community. While this might be a good answer on some site, it is not a good answer on Code Review. A good answer on Code Review contains at least one insightful observation about the code. Alternate code only solutions are considered poor answers and may be down voted or deleted by the community. Please read How do I write a good answer. \$\endgroup\$
    – pacmaninbw
    May 11 at 22:30
  • 1
    \$\begingroup\$ While this link may answer the question, it is better to include the essential parts of the answer here and provide the link for reference. Link-only answers can become invalid if the linked page changes. - From Review \$\endgroup\$
    – pacmaninbw
    May 11 at 22:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.