3

I'm making a simple server that listens for clients which is going to read the clients requests, do some calculations, send a response back to the client and the close again ASAP (somewhat similar to HTTP).

There might be many connections every seconds, so I want it to make it as fast and efficient as possible.

So far, the best way I can think of doing this, is shown as an example below:

private static ManualResetEvent gate = new ManualResetEvent(false);

static async void ListenToClient(TcpListener listener)
{
    Console.WriteLine("Waiting for connection");
    TcpClient client = await listener.AcceptTcpClientAsync();
    Console.WriteLine("Connection accepted & establised");
    gate.Set(); //Unblocks the mainthread

    Stream stream = client.GetStream();
    byte[] requestBuffer = new byte[1024];
    int size = await stream.ReadAsync(requestBuffer, 0, requestBuffer.Length);

    //PSEUDO CODE: Do some calculations

    byte[] responseBuffer = Encoding.ASCII.GetBytes("Ok");
    await stream.WriteAsync(responseBuffer, 0, responseBuffer.Length);

    stream.Close();
    client.Close();
}
static void Main(string[] args)
{
    TcpListener listener = new TcpListener(IPAddress.Any, 8888);
    listener.Start();
    while (true)
    {
        gate.Reset(); 
        ListenToClient(listener);
        gate.WaitOne(); //Blocks the main thread and waits until the gate.Set() is called
    }
}

Note: for this example and simplicity, I haven't made any error handling like try-catch and I know the response here will always be "Ok"

The code here is simply waiting for a connection, when it arrives to await listener.AcceptTcpClientAsync(), then it jumps back to the whileloop and waits until a connection is made and gate.Set() is called so it can listen for new connections again. So this will allow multiple clients at the same time (Especially if the calculations can take long time)

But should I use stream.ReadAsync() or stream.Read() instead? Im curious if it even matters because I'm already in an asynchronous function which will not block the main thread.

So my final questions are:

  1. Is this the best/right way to accomplish this task (also by using ManualResetEvent class)?
  2. Would there be any difference in this scenario to use async or non async operations when reading and writing to the stream? (because I'm not blocking the mainthread)
  3. If it lags, and takes 1-2 seconds to send/receive the data, would it still matter to choose between async and nonasync operations?

UPDATE FOR NEW IMPROVEMENTS

Due to an answer, I have updated my code to this:

private static ManualResetEvent gate = new ManualResetEvent(false);

static async Task ListenToClient(TcpListener listener)
{
    //Same Code
}
static void Main(string[] args)
{
    TcpListener listener = new TcpListener(IPAddress.Any, 8888);
    listener.Start();
    while (true)
    {
        gate.Reset();
        Task task = ListenToClient(listener);
        task.ContinueWith((Task paramTask) =>
            {
                //Inspect the paramTask
            });
        gate.WaitOne(); //Blocks the main thread and waits until the gate.Set() is called
    }
}
halfer
  • 19,824
  • 17
  • 99
  • 186
Assassinbeast
  • 1,207
  • 1
  • 17
  • 33
  • There is a great example here: http://stackoverflow.com/questions/21013751/what-is-the-async-await-equivalent-of-a-threadpool-server/21018042#21018042 – Yuval Itzchakov Jun 26 '14 at 17:11

2 Answers2

3

Right off the bat I see two common async mistakes:

async void

Don't do this. The only reason the compiler even supports async void is for handling existing event-driven interfaces. This isn't one of those, so here it's an anti-pattern. async void effectively results in losing any way of ever responding to that task or doing anything with it, such as handling an error.

Speaking of responding to tasks...

ListenToClient(listener);

You're spawning a task, but never examining its state. What will you do if there's an exception within that task? It's not caught anyway, it'll just be silently ignored. At the very least you should provide a top-level callback for the task once it's completed. Even something as simple as this:

ListenToClient(listener).ContinueWith(t =>
{
    // t is the task.  Examine it for errors, cancelations, etc.
    // Respond to error conditions here.
});
David
  • 208,112
  • 36
  • 198
  • 279
  • Hey thanks for answer! But are you sure about the ContinueWith() isn't legacy stuff? This video on youtube shows it: https://www.youtube.com/watch?v=MCW_eJA2FeY&index=3&list=PLbjPvLkw5f21ji20qbzxfOL4Zy0LzkgTW At 55:30, he says that the new async await feature will convert the stuff after await to Task.ContinueWith() – Assassinbeast Jun 26 '14 at 20:11
  • @Assassinbeast: Just because something came out in a previous version of the language doesn't make it "legacy." It's still useful. Notice that you call `ListenToClient()` synchronously without the `await` keyword. The first step of the problem is that `ListenToClient()` doesn't return a `Task`, you should fix that. Once that's fixed, you need to do something with that task, which currently you're not. If the method calling the `async` method isn't itself `async` then you can't use `await`, you need to handle the `Task` explicitly. – David Jun 26 '14 at 20:20
  • Ok, i have updated it to return a Task, so now i can inspect it. Is it this you wanted to make? If so, what if i dont care about examining the tasks state? For example, if it throws an exception, then i dont care (because i dont know what to do with it)... so the client should just be able to handle the error if it doesn't get a message back. So that way, the server can just continue its work without any disturbsion. – Assassinbeast Jun 26 '14 at 22:10
  • @Assassinbeast: It's certainly possible that in some cases you wouldn't have any meaningful way to respond to the error. But as a matter of patterns and practices you should always at least be *able* to respond. The client might handle the error in this case, or maybe they might not. Or there could start being unexpected errors that you need to debug, but if you're ignoring all errors then debugging them becomes problematic. At the very least, send them to a logging mechanism of some kind. Even if that mechanism is configured not to persist log events anywhere. – David Jun 26 '14 at 22:14
  • Ok :-) But was the new update i made in the question the way you wanted me to edit it to? – Assassinbeast Jun 26 '14 at 23:33
  • 1
    @Assassinbeast: That should work. You can also test things by throwing exceptions and seeing where/how you can catch them and respond to them. Even if in this code you don't necessarily want to, it's the overall pattern that matters. – David Jun 26 '14 at 23:34
2

Is this the best/right way to accomplish this task (also by using ManualResetEvent class)?

No. You start an async operation, then immediately wait for it. For some reason I often see this crazy dance. Just make it synchronous:

while (true) {
 var clientSocket = Accept();
 ProcessClientAsync(clientSocket);
}

So simple.

Would there be any difference in this scenario to use async or non async operations when reading and writing to the stream?

Using async IO with sockets is good if you have very many clients. For a few dozens at a time you can just use sync IO with threads. Async IO is about not blocking threads (which each use 1MB of stack space).

If you decide to use async IO, ProcessClientAsync should be an async function like you have it now.

If you decide for sync IO, start ProcessClientAsync on a new thread to be able to process multiple clients simultaneously.

If it lags, and takes 1-2 seconds to send/receive the data, would it still matter to choose between async and nonasync operations?

As long as you process individual clients independently, you are fine. The choice between sync and async only comes into play at high scale (more than dozens of connections open at the same time).

It is a common mistake to overcomplicate things by going async without the need for it. Basically all tutorials make this mistake.

usr
  • 168,620
  • 35
  • 240
  • 369