Krdytkyu

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();

        }

    }

}

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.

I've created an extension method for this which makes use of SemaphoreSlim and also allows to set maximum degree of parallelism

    /// <summary>

    /// Concurrently Executes async actions for each item of <see cref="IEnumerable<typeparamref name="T"/>

    /// </summary>

    /// <typeparam name="T">Type of IEnumerable</typeparam>

    /// <param name="enumerable">instance of <see cref="IEnumerable<typeparamref name="T"/>"/></param>

    /// <param name="action">an async <see cref="Action" /> to execute</param>

    /// <param name="maxDegreeOfParallelism">Optional, An integer that represents the maximum degree of parallelism,

    /// Must be grater than 0</param>

    /// <returns>A Task representing an async operation</returns>

    /// <exception cref="ArgumentOutOfRangeException">If the maxActionsToRunInParallel is less than 1</exception>

    public static async Task ForEachAsyncConcurrent<T>(

        this IEnumerable<T> enumerable,

        Func<T, Task> action,

        int? maxDegreeOfParallelism = null)

    {

        if (maxDegreeOfParallelism.HasValue)

        {

            using (var semaphoreSlim = new SemaphoreSlim(

                maxDegreeOfParallelism.Value, maxDegreeOfParallelism.Value))

            {

                var tasksWithThrottler = new List<Task>();



                foreach (var item in enumerable)

                {

                    // Increment the number of currently running tasks and wait if they are more than limit.

                    await semaphoreSlim.WaitAsync();



                    tasksWithThrottler.Add(Task.Run(async () =>

                    {

                         await action(item).ContinueWith(res =>

                         {

                               // action is completed, so decrement the number of currently running tasks

                               semaphoreSlim.Release();

                         });

                    }));

                }



                // Wait for all tasks to complete.

                await Task.WhenAll(tasksWithThrottler.ToArray());

            }

        }

        else

        {

            await Task.WhenAll(enumerable.Select(item => action(item)));

        }

    }

Sample Usage:

await enumerable.ForEachAsyncConcurrent(

    async item =>

    {

        await SomeAsyncMethod(item);

    },

    5);