Uses Task.Run Bad Practice?

Question

Typically, when implementing an asynchronous method in a class, I write something like this:

public Task<Guid> GetMyObjectIdAsync(string objectName) { return Task.Run(() => GetMyObjectId(objectName)); } private Guid GetMyObjectId(string objectName) { using (var unitOfWork = _myUnitOfWorkFactory.CreateUnitOfWork()) { var myObject = unitOfWork.MyObjects.Single(o => o.Name == objectName); return myObject.Id; } } 

This type of template allows me to use the same logic synchronously and asynchronously, depending on the situation (most of my work is in the old code base, and does not support asynchronous calls much), since I could expose the synchronous method publicly and get maximum compatibility if I need to.

I recently read a few SO posts that suggest using Task.Run() is a bad idea and should only be used in certain circumstances, but these circumstances did not look very clear.

Is the pattern that I pictured above a really bad idea? Am I losing some of the functionality / intended purpose of asynchronous calls by doing so? Or is it a legitimate implementation?

Answer 1

What you do is offload the synchronous operation to another thread. If your stream is "special", then that’s fine. One example of a “special” thread is a user interface thread. In this case, you can disconnect the work from it to support the user interface (another example is some kind of listener).

In most cases, however, you simply move work from one thread to another. This does not add any value and adds unnecessary overhead.

So:

Am I portrayed as a really bad idea?

Yes it is. It’s a good idea to disable synchronous operation with ThreadPool and pretend that it is asynchronous.

Am I losing some of the functionality / purpose of asynchronous calls by doing it this way?

In fact, there is nothing asynchronous in this operation. If you are doing this on a remote computer, and you can benefit from its asynchronous operation, the operation itself must be truly asynchronous, which means:

 var myObject = await unitOfWork.MyObjects.SingleAsync(o => o.Name == objectName); 

What you are doing now is called async over sync, and you probably shouldn't. Read more in Should I disclose asynchronous wrappers for synchronous methods?

Answer 2

I recently read a few SO posts that suggested using Task.Run () is a bad idea and should only be used in certain circumstances, but these circumstances do not seem very clear.

The completely naked rules of thumb that I tell people who are new to asynchrony:

First, understand the purpose. Asynchrony is designed to mitigate the important inefficiencies of high latency operations.

What do you do with low latency? Then do not make it asynchronous anyway. Just do the job. It is fast. Using a tool to reduce latency in low latency tasks just makes your program unnecessarily complicated.

Is this what you do with a long delay because it expects the disk to spin or show a packet? Make it asynchronous, but do not put it in another thread. You do not hire an employee who will be sitting at your mailbox awaiting letters; the mail system is already running asynchronously for you. You do not need to hire people to make it more asynchronous. Read “ No Topic ” if this is not clear.

Does high latency work on the processor to perform some huge calculations? How is a calculation that takes more than 10 ms? Then unload this task into the thread so that you can schedule the thread to an idle processor.

Answer 3

 public Task<Guid> GetMyObjectIdAsync(string objectName) 

When I see this, I expect it to have some advantage in using this method, and not just its wrapper in Task.Run() .

In particular, I expect it to release a stream when it gets into some I / O operations or it has the ability to do so.

Now consider if I have code:

 _resource = GetResourceForID(GetMyObjectIdAsync(SomeLongRunningWayToGetName())); 

If I have a reason for this to be done in the task, and I am in a situation where Task.Run() really makes sense (I have a reason to offload it to another thread) in the best way it would be to wrap it all up:

 Task task = Task.Run(() => _resource = GetResourceForID(GetMyObjectIdAsync(SomeLongRunningWayToGetName()))); 

Here Task.Run() may be a bad idea for me as the caller , or it may be good, because I really get what it gives me. p>

However, if I see your signature, I will think that the best way to do this with your code is to turn it into code that uses this method.

 Task task = SomeLongRunningWayToGetName() .ContinueWith(t => GetMyObjectIdAsync(t.Result)) .ContinueWith(t => _resource = GetResourceForIDAsync(t.Result)); 

(Or similarly using async and await ).

At best, this has a less good distribution of Task.Run() . In the worst case, I am await in order to benefit from better asynchrony, which he does not offer in a context that he could use if he really were there. (For example, I could use this in an MVC action, which I made asynchronous, because I thought that the extra overhead would be repaid in the best use of the thread pool).

So while Task.Run() sometimes useful, in this case it is always bad. If you cannot offer me more asynchrony than I can lead to using the class itself, do not make me think that you are doing this.

Offer only the public XXXAsync() method if it actually calls asynchronous I / O.

If you really need to align an asynchronous method, for example. matching the signature of a common base or interface, then it would be better:

 public Task<Guid> GetMyObjectIdAsync(string objectName) { return Task.FromResult(GetMyObjectId(objectName); } 

This is also bad (it would still be better for the caller to just call GetMyObjectId() directly), but at least if he is await when he is working in one thread, there is no overhead for using another thread to do the work. therefore, if mixed with another await , the negative impact will be reduced. Therefore, it is useful if you really have to return Task , but cannot add anything useful in what you call it.

But if you don't really need to, just don't do it.

(a private method that calls Run() because you have an advantage on every site and you just add convenience, rather than calling Run() in several places, but this should be well documented as such).