2
\$\begingroup\$

I'm implementing an interface with the async method Execute. In this I need to call some synchronous code. (removing an item from a dictionary). What would be the best way to go about this?

Alternative 1.

internal class ClearClaimByClaimIdCache : IListenTo<ClaimStatusChangedEvent>
{
        private readonly ICache<Guid, Claim> _cache;

        public ClearClaimByClaimIdCache(ICacheFactory cacheFactory)
        {
            _cache = cacheFactory.GetOrCreateCache<Guid, Claim>("ClaimByClaimId");
        }

        public Task Execute(ClaimStatusChangedEvent @event)
        {
            _cache.Remove(@event.ClaimId);
            return Task.CompletedTask;
        }
}

Alternative 2.

public Task Execute(ClaimStatusChangedEvent @event)
{
    return Task.Run(() => _cache.Remove(@event.ClaimId));
}

Alternative 3.

public async Task Execute(ClaimStatusChangedEvent @event)
{
    await Task.Run(() => _cache.Remove(@event.ClaimId));
}

Or any better way?

Improve this question
\$\endgroup\$
  • \$\begingroup\$ Scratch what I said - I'd say alternative 2. If you write something that can be asynchronous, it should always be asynchronous. Similarly, methods that are expected to be synchronous should always execute synchronously. So I'd personally say Alternative 2. Alternative 3 is kind of silly because it creates a state machine for no reason (that's what await does). If you were composing multiple actions inside of that method then I'd maybe say yes, use await, but otherwise just return the Task. \$\endgroup\$ – Dan Nov 5 '15 at 11:03
  • \$\begingroup\$ Thanks for your input @DanPantry. But wont we start an unnecessary thread using alternative 2? In alternative 1 the static CompletedTask is used and no new thread is needed. I agree with you about alternative 3. \$\endgroup\$ – Magnus Nov 5 '15 at 12:17
  • \$\begingroup\$ I'm not sure if tasks create new threads (pretty sure they work on an internal thread pool that is already allocated), however that's just a downside with asynchrony. To make your code easier to reason about, any code that may return asynchronously must. Here is an article on it from the Node ecosystem, and explains the reasons why I take this stance. blog.izs.me/post/59142742143/designing-apis-for-asynchrony \$\endgroup\$ – Dan Nov 5 '15 at 12:19
  • \$\begingroup\$ @DanPantry Having synchronous Task-returning method is not the same as calling a callback synchronously. For example, the Task method returns before the caller continues, so it won't have anything on the call stack. \$\endgroup\$ – svick Nov 5 '15 at 12:52
  • \$\begingroup\$ @svick fair enough, I am unsure of the dynamics of how tasks work in C#, my point being that if your API relies on being asynchronous then it should always be asynchronous. \$\endgroup\$ – Dan Nov 5 '15 at 12:53
7
\$\begingroup\$

If it's really just removing an item from a dictionary (and not e.g. doing synchronous file IO), then Alternative 1 is the best.

When you're using async for scalability (e.g. in ASP.NET), using Task.Run() like this won't help you (since the number of threads used stays the same), it will only hurt you a bit (since Task.Run() has some overhead).

When you're using async for responsiveness (e.g. in GUI applications), using Task.Run() like this can make sense, but only for long-running operations (MS recommendation is operations that can take more than 50 ms). Since dictionary operations shouldn't take long you shouldn't use Task.Run() here either.

Improve this answer
\$\endgroup\$

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.