current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

Say I have an interface and I want to expose both synchronous and asynchronous options for performing an action:

public interface MyInterface
{
   Task<SomeResult> FetchSomeThingsAsync();
   SomeResult FetchSomeThings();
}

Is it appropriate for the underlying implementation of FetchSomeThings to be:

public SomeResult FetchSomeThings()
{
   // Handle unwrapping AggregateException, if set
   return FetchSomeThingsAsync().Result;
}

My gut is telling me that this isn't a great idea, but I'd really like to put my finger on why. My initial thoughts:

  1. this will actually use two threads, the blocked calling thread and a TPL thread
  2. this may be violating a caller's expectations for a synchronous method

My question: Is this bad? Does it have the potential to impact callers of the interface in a negative way? Are there issues with "correctness", is it simply wasteful from a performance standpoint, or is it even a good idea since it results in simpler code?

EDIT: Moving this from a comment I left below:

I frequently deal with cases where I use lower-level async I/O methods in my async implementation. Ex. The asynchronous version may need to call, for example, ExecuteRequestAsync() on some lower level library, rather than simply wrapping the synchronous version in a task (like I might do for compute-bound operations). This is what has, historically, led me away from the opposite pattern -- implementing asynchronous methods by leveraging their synchronous counterparts.

To clarify the question a bit, lets assume that the asynchronous version requires a slightly more complex implementation than simply wrapping it in a Task.Run.

share|improve this question
1  
1) IO usually doesn't block a TPL thread during the operation itself. The OS will send a notification once the IO operation completes, and only then the operation gets scheduled on a TPL thread. 2) How does this violate "caller's expectations for a synchronous method"? –  CodesInChaos Apr 2 at 11:09
    
Thanks. The clarification in point 1 helps. 2 was more or less my question -- nothing stands out to me, but I was open to the possibility that there was something I was overlooking. Based on this, I'm hearing that this seems like a perfectly fine implementation for I/O bound operations, and Uri Agassi's answer is a more appropriate implementation for compute bound operations. –  joshtkling Apr 2 at 13:01
2  
There is one subtle point: You need to avoid scheduling something on the original during the async operation. So you might need to use ConfigureAwaiter if you use asnyc-await. –  CodesInChaos Apr 2 at 13:07

2 Answers 2

up vote 3 down vote accepted

First, Stephen Toub has a good pair of articles about this topic: Should I expose asynchronous wrappers for synchronous methods? and Should I expose synchronous wrappers for asynchronous methods?

The gist of the articles is that you shouldn't do either: keep your asynchronous operations asynchronous and keep the synchronous operations synchronous (at least at the library level).

Now, probably the biggest problem with using Result (which is also mentioned in the article) is that it can deadlock, if the async method tries to resume in the original context. This happens most commonly when you use await without ConfigureAwait(false).

To make sure this doesn't happen, you have to use ConfigureAwait(false) for every await in the asynchronous version of your method (and the asynchronous methods it calls).

share|improve this answer
    
Thank you -- this was very helpful, and those articles are exactly the kind of deeper dive I was looking for on this subject. –  joshtkling Apr 6 at 14:33
up vote 3 down vote

I believe the opposite approach is better - implement the synchronous FetchSomeThings(), and then wrap it with asynchronous execution in FetchSomeThingsAsync():

public Task<SomeResult> FetchSomeThingsAsync() 
{
    Task.Run(()=>FetchSomeThings());
}

This way only relevant resources will be used for each use-case.

share|improve this answer
    
So, one thought on that. I frequently deal with cases where I use lower-level async I/O methods in my async implementation. Ex. The asynchronous version may need to call, for example, ExecuteRequestAsync() on some lower level library, rather than simply wrapping the synchronous version in a task (like I might do for compute-bound operations). This is what has, historically, led me away from things like this. –  joshtkling Mar 28 at 13:30
2  
This will unnecessarily tie up a thread during the whole IO operation even while using the async API. –  CodesInChaos Apr 2 at 11:10

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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