4
\$\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\$
11
\$\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\$
  • \$\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\$ – Raskolnikov Feb 12 '18 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 '18 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\$ – Raskolnikov Feb 12 '18 at 12:08
  • \$\begingroup\$ @Raskolnikov as I said before event handlers are the one exception (pardon the pun) to that rule. async void on event handlers are allowed. Read the reference link in the answer. \$\endgroup\$ – Nkosi Feb 12 '18 at 12:09
  • \$\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 '18 at 14:50

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.