0
\$\begingroup\$

I have a method that takes a collection of objects, and in turn calls a method that handles a single object. My question is, do I need to handle the tasks coming from the DeleteSingleAsync method since it's not really an async method or is this safe since the concrete implementation is just returning a completed task.

My initial gut concern is with exceptions/cancelation tokens not being handled correctly, or a slim possibility of a race condition in the tableTransactionActions object.

private readonly IList<TableTransactionAction> tableTransactionActions;


public Task DeleteSingleAsync(TEntity entity, CancellationToken cancellationToken = default)
{
    cancellationToken.ThrowIfCancellationRequested();
    tableTransactionActions.Add(new TableTransactionAction(TableTransactionActionType.Delete, entity));
    return Task.CompletedTask;
}

public Task DeleteAsync(IEnumerable<TEntity> entities, CancellationToken cancellationToken = default)
{
    foreach (var entity in entities)
    {
        DeleteSingleAsync(entity, cancellationToken);
    }
    return Task.CompletedTask;
}
\$\endgroup\$
4
  • 5
    \$\begingroup\$ Why is your API async if your implementation is synchronous? What do you mean by this: cancelation tokens not being handled correctly? \$\endgroup\$ Jun 1, 2022 at 6:53
  • \$\begingroup\$ It's implementing an interface. \$\endgroup\$
    – Russ
    Jun 1, 2022 at 10:39
  • 2
    \$\begingroup\$ Okay. What about the other question? \$\endgroup\$ Jun 1, 2022 at 10:41
  • \$\begingroup\$ are you trying to create a list of tasks, which would be executed whenever a certain method called ? (something like SaveChangesAsync()). ? \$\endgroup\$
    – iSR5
    Jun 1, 2022 at 20:48

1 Answer 1

2
\$\begingroup\$

Not waiting in DeleteAsync should be safe but I wouldn't pass it from a code review perspective as in the future if DeleteSingleAsync changes to be truly async and someone doesn't know they also need to change DeleteAsync bugs could come out of it. This is coding to the implementation and not the interface, which is bad practice IMO. We still don't need to await it in DeleteAsync just do

return Task.WhenAll(entities.Select(entity => DeleteSingleAsync(entity, cancellationToken))

Now in the future if we change DeleteSingleAsync to be async DeleteAsync is still good to go.

If tableTransactionActions is a standard C# List<> then it is not thread safe. Without knowing more of how this is used outside of this small snippet I would suggest to use the ConcurrentQueue, or possibly ConcurrentStack, instead of List. But to know which Thread Safe class to use would need more information then the current code shown. Thread safety isn't about async it's about being called in parallel from multiple threads at the same time.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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