Tcp Server performance issues after implementing thread pool

Question

I have read several discussion on the matter and managed to understand a bit, but still having some issues solving the performance problem when converting from Thread class to ThreadPool class

Details:

I have build a tcp server, with educational purpose(the task should not be with async methods), which is accepting client connections and create a new thread for each client. With this method the application is executed for less than a second, but when decided to move to a next level solution like thread pool my performance dropped to 40-50 seconds for just a hundred clients and i just send a 2048 byte buffer, receive it and close.

The first 12 threads are very fast, most probably because my cpu is 6 cores 12 threaded and after that the threads start to experience a delay. I am open to solution ideas and structure approach.

Server code:

public void OnSocketReceive()
    {
        while (!this.exitServer)
        {
            if (!tcpListener.Pending())
            {
                Thread.Sleep(10);
                continue;
            }

            TcpClient client = tcpListener.AcceptTcpClient();

            IManageConnectedUser chatLogic = new ManageConnectedUser(connections, welcomeMessage);

            //Thread clientThread = new Thread(new ParameterizedThreadStart(chatLogic.OnClientConnection));
            //clientThread.Start(client);

            ThreadPool.QueueUserWorkItem(chatLogic.OnClientConnection, client);
        }

More clarification

By far with the debugging I have done and the discussions I have read I made the conclusion that the problem is the blocking code in OnClientConnection function and more specifically in the inner function which is stringCreateHandler. Where i receive this error: System.Threading.ThreadInterruptedException: Thread was interrupted from a waiting state on line 39, which is the Thread.Sleep(10);

public string stringCreateHandler(TcpClient client)
    {
        StringBuilder sb = new StringBuilder();
        try
        {
            do
            {
                if (client.Available > 0)
                {
                    while (client.Available > 0)
                    {
                        char ch = (char)client.GetStream().ReadByte();

                        if (ch == '\r')
                        {
                            continue;
                        }
                        if (ch == '\n')
                        {
                            return sb.ToString();
                        }

                        sb.Append(ch);
                    }
                }

                Thread.Sleep(10);
              
            } while (true);
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
            throw;
        }

    }

I have checked the rest of the code i don't think there is more blocking code, but i am giving a link to the project https://github.com/nikolaymih/Chat-Project/tree/master/ChatProjectNewThreads The only difference there is the Thread class instead of ThreadPool but this is just for orientation

'create a new thread for each client', ok, so why are you using polling loops instead of blocking? — Martin James, Jul 06 '21 at 08:57
Why aren't you using async methods? Async methods use the threadpool too, in a far more efficient manner. And blocking a threadpool thread with `Thread.Sleep` is very expensive - it means that a shared thread is no longer able to serve requests. — Panagiotis Kanavos, Jul 06 '21 at 08:57
@PanagiotisKanavos because the task is not to use async methods. What is the alternative for thread.sleep in this case — Nikolay, Jul 06 '21 at 09:06
To use async methods, and process completion instead of polling. Something easily done with `await`. It makes no sense saying `the task is not to use async methods`. Windows is an asynchronous OS. All IO is asynchronous, blocking operations are *emulated*. Actual IO doesn't use a threadpool thread, it's performed *asynchronously* at the driver level. Extra operations at the application level are performed by a different IO threadpool. Kestrel is so fast because it doesn't block. — Panagiotis Kanavos, Jul 06 '21 at 09:09
At the Win32 API level, where `await` isn't available, asynchronous IO operations will call a callback method when an IO operation completes. You can do the same with the old-style BeginXYZ/EndXYZ programming model, eg `BeginAcceptTcpClient/EndAcceptTcpClient`. `async/await` makes this a lot easier though. — Panagiotis Kanavos, Jul 06 '21 at 09:11
You can go even faster if you use [IO pipelines](https://devblogs.microsoft.com/dotnet/system-io-pipelines-high-performance-io-in-net/) instead of streams, passing shared buffers between producers and consumers (the socket and the threads that process the data). That's what Kestrel uses right now. That API is fully async though — Panagiotis Kanavos, Jul 06 '21 at 09:14
@MartinJames at the beginning i was using a blocking loops with the new threads, but the idea was just to see how it works and that the are not a good idea. After that i moved to thread pool with the polling loop — Nikolay, Jul 06 '21 at 09:14
It's horrid code to write `catch (Exception e) { Console.WriteLine(e); throw; }` - you should remove this and never code like this again. — Enigmativity, Jul 06 '21 at 09:34
Nothing wrong with the "log and rethrow" pattern... Logging is normally more nuanced than just printing to console, but this is a toy app — canton7, Jul 06 '21 at 09:57
You say `the task is not to use async methods` but you also say `I am open to solution ideas and structure approach` these two things are contradictory. The correct way to do this in modern programming is to use `async`, so you are going to need to explain why you don't want to use it — Charlieface, Jul 06 '21 at 10:05
@canton7 - Sure there is - it's an anti-pattern design to swallow exceptions and hide them from the programmer. Only specific exceptions that can be meaningfully handled should be caught. Otherwise, if there is a "fail and bail" requirement then that should be further up the call stack. — Enigmativity, Jul 06 '21 at 11:06
@Enigmativity OP re-throws the exception though -- it's not swallowed. — canton7, Jul 06 '21 at 12:09
@Enigmativity `throw e;` would lose it, but `throw;` does not — canton7, Jul 06 '21 at 12:11
@canton7 - And just adds lines of code that make your code harder to read and harder to refactor. It leads to fragile coding. — Enigmativity, Jul 06 '21 at 12:11
@canton7 - Further reading on why exceptions suck - https://ericlippert.com/2008/09/10/vexing-exceptions/ — Enigmativity, Jul 06 '21 at 12:13
@Enigmativity This is how C# has been since time immemorial. It is a shame that it doesn't support a `fault` handler, this acts like a `finally` but only runs in event of an exception, and is basically the same as `catch{ throw; }` — Charlieface, Jul 06 '21 at 16:39
@Charlieface - You talk as if you must use exception handlers. The fewer cases that you need to use them the stronger your code is. — Enigmativity, Jul 06 '21 at 23:58
@Enigmativity I meant that this how you do exception logging in C#. Of course you don't have to, but a statement like `client.GetStream` it's common to have some kind of exception logging and/or handling. And if you'd read that article from Eric, you would have seen he says to catch all "exogenous" exceptions, such that you often get from reading a stream — Charlieface, Jul 07 '21 at 09:54

canton7 · Accepted Answer · 2021-07-06T09:14:38.733

3

Looking at your full project, I think your issue is that OnClientConnection is long-running: it won't return until that connection is closed, and since this is a "chat app", connections stay open and aren't closed quickly.

As you've seen, the ThreadPool starts off with a number of idle threads, and it will add threads as needed at a rate of approximately one every 500ms (although this is an implementation detail). This normally isn't a problem: you're not supposed to use the ThreadPool for very long-running tasks which never return, and which tie up threads forever. However, since your OnClientConnection method doesn't return for ages, you're just grabbing and hogging ThreadPool threads, and forcing the ThreadPool to keep expanding.

If you want to use a thread per client, you are probably best off creating a new long-running thread per client. However, this is pretty wasteful: you'll have loads of threads hanging around not doing very much. It's better to have a small number of threads which are constantly processing messages from a big pool of clients. It's possible to architect this yourself, but it's a lot easier to just use the async/await capabilities of TcpListener and NetworkStream.

edited Jul 06 '21 at 09:14

answered Jul 06 '21 at 09:09

canton7

37,633
3
64
77

Thanks! I agree with the things you said, especially that i am holding a thread forever in that function. I was hoping that there is a decision without changing the whole structure and using thread pool but now i see it is inevitable. I prefer not to use async/await because of my task and i will try to architect this by myself. If you have any useful resources i will be grateful – Nikolay Jul 06 '21 at 09:43
@Nikolay you could try adding this line at the start of the program: `ThreadPool.SetMinThreads(1000, 1000);`. This way the `ThreadPool` will immediately create new threads on demand, and will not get saturated until it has reached the threshold of 1,000 threads (or whatever number you choose to configure it). Be aware that each thread consumes at least [1 MB](https://stackoverflow.com/questions/28656872/why-is-stack-size-in-c-sharp-exactly-1-mb) of RAM, so make sure that you have a ton of RAM available before deciding to go that route. – Theodor Zoulias Jul 06 '21 at 11:54
A 'long running' thread is not particularly wasteful. If it does not run often, it's loaded stack pages, TCB etc. will get swapped out and it will exist only as a pointer in some OS container. – Martin James Jul 08 '21 at 08:10

Tcp Server performance issues after implementing thread pool

1 Answers1