CODEWITHSUNDEEP
c#asynchronousasync-awaitthreadpool
Jim W
Jim W

Reputation: 5026

Is Task.Run appropriate here, wrapping network calls?

I'm studying a pretty popular c# proxy server. In it there is code such as (I'm hiding the proxy server identity on purpose because I feel bad telling tales, if indeed it is wrong).

private void OnConnectionAccepted(TcpClient cl)
{
     ....
     Task.Run(async () =>
            {
               await HandleClientRequest();

            });
     Task.Run(async () =>
        {
            TcpClient cl = await listener.AcceptTcpClientAsync();
            OnConnectionAccepted(cl);
        });
}

Where HandleClientRequest does some async network calls, such as reading the request from the client and repeating it to the server, like you'd expect a proxy to do. listener.AcceptTcpClientAsync just waits for the next connection to come from a browser.

So it seems the point of the Task.Run is really just to allow AcceptTcpClientAsync to be called while HandleClientRequest is busy.

This seems to go against the recommendations I've been reading, eg. http://blog.stephencleary.com/2013/10/taskrun-etiquette-and-proper-usage.html because Task.Run is wrapping network, not CPU calls.

So how should it work instead, if this is wrong, should it use ThreadPool.QueueUserWorkerItem?

The context by the way would typically be a console app.

Upvotes: 1

Views: 914

Answers (2)

Stephen Cleary
Stephen Cleary

Reputation: 457302

This seems to go against the recommendations I've been reading... because Task.Run is wrapping network, not CPU calls.

In general, Task.Run should not be used for I/O. However, that advice is intended for application code, and even there has several exceptions.

In this case (examining the core accept/process logic of a proxy server), the code we're looking at is more of a framework, with the "application" being inside HandleClientRequest. (Note that this code is hosted in a Console or Win32 service, not within ASP.NET).

To draw a parallel, ASP.NET will listen for connections and will take a thread from the thread pool to process requests. This is perfectly natural for a framework to do.

Here are some reasons to use Task.Run, that may or may not be valid for this particular case:

  • Some .NET network calls start with a synchronous portion - in particular, HTTP proxy and DNS lookups are done synchronously. This is very unfortunate, but we're stuck with it for backwards compatibility reasons. So even asynchronous network APIs can be partially synchronous. If HandleClientRequest makes use of these APIs, it would make sense to wrap it in a Task.Run.
  • The code may not want the current SynchronizationContext. This doesn't appear to be the case here, though.
  • All asynchronous methods begin executing synchronously. If HandleClientRequest does some amount of "housekeeping" before it gets down to its real work, it may be beneficial to wrap it in a Task.Run so that the listener is not blocked.
  • Recursive asynchronous code can fill the stack if it executes synchronously. E.g., if the second Task.Run wasn't present and a lot of connections were made all at once, then the stack could overflow. I assume this is the purpose of the second Task.Run because I can't think of any other purpose it would have.

So how should it work instead, if this is wrong, should it use ThreadPool.QueueUserWorkerItem?

Absolutely not! Task.Run is the appropriate way of using the thread pool. (Unless you have an extremely smart team and you can show a measurable performance difference).

So it seems the point of the Task.Run is really just to allow AcceptTcpClientAsync to be called while HandleClientRequest is busy.

The usage of Task.Run is still questionable at best, because the tasks should be awaited sooner or later. As it currently stands, any exceptions from HandleClientRequest, AcceptTcpClientAsync, and OnConnectionAccepted will be silently dropped. If HandleClientRequest and OnConnectionAccepted both had top-level try blocks, that would be OK, but any exceptions from AcceptTcpClientAsync would cause the whole proxy server to stop working. And that method can throw exceptions, though it is rare.

Upvotes: 5

Thomas Zeman
Thomas Zeman

Reputation: 910

I would propose refactoring this a little bit into one method accepting connections and another one dealing with incoming data. Finally a third one dealing with failed tasks.

public async Task Acceptor(TcpListener listener)
{
  for (;;)
  {
    TcpClient client = await listener.AcceptTcpClientAsync();
    OnConnectionAccepted(client).ContinueWith(task => { errorHandling }, OnlyOnFaulted);
    // some logic which will stop this loop if necessary
  }
}

private async Task OnConnectionAccepted(TcpClient client)
{
  await HandleClientRequest(client);
  // .. further logic
}

"Acceptor" will throw when the listener cannot accept new connections anymore. "errorHandling" will be called when one particular connection cannot be processed. (You can also consider a try catch in OnConnectionAccepted and making it async void - which I personally would not prefer)

Stephen is right, there is hardly any case where Task.Run should be used. That is also because async/await and Task.Run work on different abstraction levels. Unless you are writing a library of some kind async/await should be always enough.

Upvotes: 1

Related Questions