3

Is there a better way to write this asynchronous code (e.g. so that I don't need to repeat if (myCondition) twice)? I want to avoid using Task.Run here.

var tasks = new List<Task>();
Task<String> t1 = null;
Task<String> t2 = null;

if (myCondition) {
    t1 = getAsync();
    tasks.Add(t1);
}

if (myOtherCondition) {
    t2 = getAsync2();
    tasks.Add(t2);
}

await Task.WhenAll(tasks)

if (myCondition) {
    result.foo = await t1;
}

if (myOtherCondition) {
    result.bar = await t2;
}
jmn
  • 889
  • 1
  • 8
  • 25
  • If there are no interdependencies between t1 and t2, I'm not sure why you'd do this at all. – Erik Philips Oct 02 '21 at 12:38
  • Hi! The idea is to await the tasks in parallel with `Task.WhenAll()` – jmn Oct 02 '21 at 12:51
  • Replace the second set of checks with `if (t1 != null) {}` etc. – GSerg Oct 02 '21 at 13:08
  • Thanks @GSerg! Yeah I'll do that. Curious if this could be further improved. – jmn Oct 02 '21 at 13:13
  • 1
    You also do not await Task.WhenAll so it basically does nothing (not that it's needed anyway). – Evk Oct 02 '21 at 13:15
  • 1
    @jmn Well, I guess you can have a `List<(Task, Action)>`, store things like `(getAsync(), (s, r) => {r.foo = s;})` in there, and then you can get rid of your `t1` and `t2` and just process the list executing the lambda passing it the result of the tupled task, but it's probably arguable whether it's an improvement. You wouldn't need `WhenAll`, just `await` the tasks in the order they are in the list, they will still be in flight concurrently - but this is also the case with your current code like noted in above comments, so no improvement here. – GSerg Oct 02 '21 at 13:31
  • Really, just keep it simple. No need for list, no need for `Task.WhenAll`. Just check task for null as suggested above before `... = await task;` and that's it. – Evk Oct 02 '21 at 14:17
  • @GSerg omitting the `await Task.WhenAll` is not a good idea, because in case of failure you may end up with fire-and-forget tasks. – Theodor Zoulias Oct 02 '21 at 17:50
  • @TheodorZoulias If you started any tasks at all, they will be in the list. You will obviously await each task in the list individually before calling the lambda. There is no task that is not awaited. If you are saying an exception may happen before all tasks are created, then same exception will prevent you from `WhenAll`, and if you handle such exceptions properly so that you get to `WhenAll` in any case, that same handling will allow you to arrive to the waiting of each task. – GSerg Oct 02 '21 at 19:13
  • @GSerg if you `await` each task individually and sequentially, and one of the `await` fails, then (unless I am missing something) the rest `await`s will be skipped, and the associated tasks will become fire-and-forget. Which is a problem that can be avoided by simply not omitting the `await Task.WhenAll`. – Theodor Zoulias Oct 02 '21 at 19:25
  • @TheodorZoulias Obviously you would somehow handle exceptions when awaiting. You would need to do that after `WhenAll` anyway. – GSerg Oct 02 '21 at 19:29
  • @GSerg could you post somewhere (on https://dotnetfiddle.net/ for example) a minimal demo showing how you can handle the exceptions of multiple sequential awaiting, in a way that prevents any of the associated tasks to become fire-and-forget? – Theodor Zoulias Oct 02 '21 at 19:33

2 Answers2

1

Without really knowing what your conditions are checking, I think I would usually move that check to within the method that actually relates to getting foo or bar. It seems like a case of your one method doing more than it's supposed to.

A different approach:

var fooTask = GetFoo();
var barTask = GetBar();

await Task.WhenAll(new [] { fooTask, barTask });
result.foo = (await fooTask) ?? result.foo;
result.bar = (await barTask) ?? result.bar;

// ...
async Task<string> GetFoo()
{
    if (!myCondition) {
        return Task.FromResult((string)null);
    }
    return await DoHeavyWorkFoo();
}

async Task<string> GetBar()
{
    if (!myOtherCondition) {
        return Task.FromResult((string)null);
    }
    return await DoHeavyWorkBar();
}
Xerillio
  • 4,855
  • 1
  • 17
  • 28
  • This is an interesting approach, but is assumes that setting the `foo` and `bar` properties to `null` is OK. This may not be the case. These properties may have already been initialized to non-`null` values, during the construction of the `result` object. – Theodor Zoulias Oct 02 '21 at 17:58
  • 1
    @TheodorZoulias Agreed, but if that's true, I suspect there are some design flaws. But that's just guess-work based on this very hypothetical example. I'll adjust the code the handle that with `(await fooTask) ?? result.foo` – Xerillio Oct 02 '21 at 18:34
0

One way to do this is to have two lists, one list of tasks and one list of actions. The actions will be invoked sequentially after the completions of all tasks, and will assign the properties of the result object. Example:

var tasks = new List<Task>();
var actions = new List<Action>();
var result = new MyClass();

if (myCondition)
{
    var task = getAsync();
    tasks.Add(task);
    actions.Add(() => result.Foo = task.Result);
}

if (myOtherCondition)
{
    var task = getAsync2();
    tasks.Add(task);
    actions.Add(() => result.Bar = task.Result);
}

await Task.WhenAll(tasks);

actions.ForEach(action => action());

This way you don't need to store each Task in a separate variable, because each lambda expression captures the task variable in the inner scope of the if block. When the Action is invoked, the task will be completed, and so the task.Result will not block.


Just for fun: If you want to get fancy, you could encapsulate this "parallel object initialization" functionality in an ObjectInitializer class, that would invoke all the asynchronous methods concurrently, and then create a new object and assign the value of each of its properties sequentially:

public class ObjectInitializer<TObject> where TObject : new()
{
    private readonly List<Func<object, Task<Action<TObject>>>> _functions = new();

    public void Add<TProperty>(Func<object, Task<TProperty>> valueGetter,
        Action<TObject, TProperty> propertySetter)
    {
        _functions.Add(async arg =>
        {
            TProperty value = await valueGetter(arg);
            return instance => propertySetter(instance, value);
        });
    }

    public async Task<TObject> Run(object arg = null)
    {
        var getterTasks = _functions.Select(f => f(arg));
        Action<TObject>[] setters = await Task.WhenAll(getterTasks);
        TObject instance = new();
        Array.ForEach(setters, f => f(instance));
        return instance;
    }
}

Usage example:

var initializer = new ObjectInitializer<MyClass>();
if (myFooCondition) initializer.Add(_ => GetFooAsync(), (x, v) => x.Foo = v);
if (myBarCondition) initializer.Add(_ => GetBarAsync(), (x, v) => x.Bar = v);
MyClass result = await initializer.Run();
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • @jmn I added a "just for fun" approach. It's a modified version of a similar class that I've posted in [this](https://stackoverflow.com/questions/69140421/updating-different-properties-of-same-object-in-parallel-invoke-is-thread-safe "Updating different properties of same object in Parallel.Invoke") question a few weeks ago. – Theodor Zoulias Oct 02 '21 at 15:25
  • Why did you make it two lists, instead of one list of tuples? – GSerg Oct 02 '21 at 15:26
  • @GSerg sure, you could use one list instead of two, but I am not sure if this would make the code more readable or more efficient. – Theodor Zoulias Oct 02 '21 at 16:36
  • It certainly would, for the same reason why an array of `struct {int id, string name}` is more maintainable than two arrays of `int[] ids` and `string[] names`. – GSerg Oct 02 '21 at 19:11
  • @GSerg hmm, could you post the one-list implementation as an answer, so that we can compare visually the two implementations? – Theodor Zoulias Oct 02 '21 at 19:27
  • That would be https://stackoverflow.com/questions/69416885/avoid-checking-condition-twice-when-assigning-from-task-awaited-with-task-whenal/69417690?noredirect=1#comment122694841_69416885. It's not so much about the visuals, but about the fact that two separate arrays do not by themselves convey the fact that they are two inseparable parts on a whole and must be moved around/changed/resized only together should the need arise. – GSerg Oct 02 '21 at 19:31
  • @GSerg in principle I agree with what you are saying. But for the purpose of providing an easily digestible answer to the question asked, using two arrays results to a simpler example IMHO. If you noticed I have updated my answer, and I've included an advanced solution that encapsulates fully the data and metadata associated with this problem. But it wouldn't make for a good answer as is, without the original simple/educational example. – Theodor Zoulias Oct 02 '21 at 19:45
  • @TheodorZoulias Would it be a possible to write `actions.Add(async () => result.Bar = await task);` for your first answer, just to habitually stay clear of Task.Result or is this a bad idea? – jmn Oct 02 '21 at 20:05
  • @jmn it's a bad idea. The lambda `async () => result.Bar = await task` when assigned to an `Action` becomes [`async void`](https://learn.microsoft.com/en-us/archive/msdn-magazine/2013/march/async-await-best-practices-in-asynchronous-programming#avoid-async-void), because an `Action` returns void, and so the asynchronous lambda cannot return a `Task`. – Theodor Zoulias Oct 02 '21 at 20:12