CODEWITHSUNDEEP
c#multithreadingasynchronousthreadpool
sosil
sosil

Reputation: 389

Determining how to properly queue work when network client connects

I have been reading about Threadpool.QueueUserWorkItem (what I've been using), Task.Run and Task.Factory.StartNew and am still unclear as to what the right option is here. It seems since my work is not CPU bound, I should probably not use Task.Run

I have my own thread running that waits for connections:

Thread listenThread = new Thread(() => ListenForConnection());
listenThread.Name = "lThread";
listenThread.IsBackground = true;
listenThread.Start();

and inside that I am simply doing:

tlist = new TcpListener(IPAddress.Parse(ip.ToString()), 27275);
tlist.Start();

while (isRunning) {
     try {
          var client = await tlist.AcceptTcpClientAsync();
          ThreadPool.QueueUserWorkItem(HandleClient, client);
     }
     catch (Exception) { }
}

HandleClient parses the message the client sends, and either creates an instance of a simple class or updates an existing one if the client already exists, stores references to the connection, and also updates a few UI elements.

Is ThreadPool.QueueUserWorkItem the preferred method here or am I way off?

Edit: just to note, the HandleClient function usually takes between 5 and 30 ms, so not very heavy work

Upvotes: 1

Views: 37

Answers (2)

usr
usr

Reputation: 171226

QueueUserWorkItem, StartNew and Run are more or less equivalent. I'd pick Task.Run because it is the most modern way to do it.

The way you have done it is totally OK. You have chosen a synchronous HandleClient implementation which forces you to handle the connection on a separate thread. Note, that this can consume many resources in case of many concurrent connections. If you are not interested in that scenario this is not a problem. Otherwise, it is a dealbreaker.

In case you make HandleClient truly asynchronous and non-blocking you do not need to push that call onto the thread-pool. My advice is to still do that because it has very little downside and it protects you against an overly long synchronous initial part of that method.

catch (Exception) { } this I don't understand. This might hide bugs. Also, if there is an error accepting most likely it is a permanent error. This kind of error handling will turn the loop into a busy loop consuming 100% of one CPU code. Move the catch outside of the loop.

Upvotes: 1

user6522773
user6522773

Reputation:

I would say like this:

var tcpListener = new TcpListener(IPAddress.Any, 80);
tcpListener.Start();
while (true)
{
  var tcpClient = tcpListener.AcceptTcpClient();
  Task.Factory.StartNew(() =>
  {
    // Do whatever you like with your TcpClient
  });
}

Upvotes: 0

Related Questions