1

I am making a simple TCP server and stumbled on this article by Marc Gravell mentioning the following:

Use ValueTask[], unless you absolutely can't because the existing API is Task[], and even then: at least consider an API break

So I changed my HandleClient() method to return ValueTask rather than Task:

public async Task Start()
{
    TcpListener.Start();
    Console.WriteLine($"Server started. Listening on {ServerIP}:{ServerPort}");

    while (true)
    {
        try
        {
            TcpClient client = await TcpListener.AcceptTcpClientAsync();
            _ = HandleClient(client);
        }
        catch (Exception ex)
        {
            Console.WriteLine($"Error accepting client: {ex.Message}");
        }
    }
}

private async ValueTask HandleClient(TcpClient client) 
{
    ...
    int bytes = await stream.ReadAsync(buffer, cts.Token);
    ...
    await stream.WriteAsync(response);
    ...
}

After changing the return type to ValueTask though, I am getting the following warning: CA2012: ValueTask instances returned from method calls should always be used, typically awaited. Not doing so often represents a functional bug, but even if it doesn't, it can result in degraded performance if the target method pools objects for use with ValueTasks.

However, I cannot use await here because the while loop has to keep running to accept connections concurrently while HandleClient() executes on a different thread. From this github issue, it is suggested to use HandleClient().Preserve(), but doesn't that defeat the purpose of changing my method to ValueTask since according to MSDN documentation:

You can use the Preserve() method to convert this ValueTask into the instance backed by a regular Task that is safe to await multiple times.

So, if I use Preserve() then I am basically using a regular Task from what I understand, and I might as well just change HandleClient() back to return Task instead.

I also tried assigning it to a variable and doing so does get rid of the warning, but I don't know if that solves the problem (if it even exists).

var t = HandleClient(client);

This assignment also does not give me the regular warning CS0129: The variable is assigned but its value is never used, so maybe that is actually the solution?

Moaaz Assali
  • 147
  • 1
  • 8
  • Per this link: [C# Async Task Method Without Await or Return](https://stackoverflow.com/questions/47187544/c-sharp-async-task-method-without-await-or-return): "You only mark a method as async if you're planning on using an await (directly) inside of it. The fact that it returns a Task is enough to satisfy the interface; async/await are irrespective of interfaces." So just remove the "async", remove the "var t =" ... and you should be good to go :) – paulsm4 Jul 25 '23 at 18:59
  • I didn't include this in my post, but my HandleClient() method uses await inside by calling await stream.ReadAsync(buffer) – Moaaz Assali Jul 25 '23 at 19:05
  • OK: if you actually *NEED* "await" in the *SAME* method, then another approach might be to [suppress the CA2012 warning](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings) for that particular module. – paulsm4 Jul 25 '23 at 19:08
  • 2
    This is a question that @MarcGravell should answer, since he is the culprit of the [original article](https://blog.marcgravell.com/2019/08/prefer-valuetask-to-task-always-and.html). :-) – Theodor Zoulias Jul 25 '23 at 19:09
  • 2
    PS: It sounds like plain old "Task" is perfectly OK in this case :) – paulsm4 Jul 25 '23 at 19:18
  • Yeah, I will probably just switch back to Task now. I just decided to use it since @MarcGravell recommended it as the default choice basically. Also it's not like my server is handling thousands of requests per second to benefit from any performance boost using ValueTask lol. – Moaaz Assali Jul 25 '23 at 19:23
  • Have you considered changing the return type to `void`? – Ben Voigt Jul 25 '23 at 19:51

0 Answers0