Client disconnection and reconnection in C# NamedPipeServerStream

Question

I have written a C# .NET code that utilizes NamedPipeServerStream to send and receive data. My goal is to ensure that when a client disconnects, the server waits and then establishes a new connection when the client reconnects.

The code seems to work in my initial tests, but since I'm new to this, I'm unsure if I've implemented it correctly. my main concern is with memory leaks, performance, and the way I handle Named Pipes recovery when a client disconnects.

I would greatly appreciate any suggestions or insights on whether this is the proper way to achieve this.

Here's the code I've written:

using System.IO.Pipes;
using System.Text;

namespace Service.IPC;

public class PipeServer
{
    private NamedPipeServerStream _pipeServer;
    private readonly string _pipeName;
    private StreamReader reader;

    private Thread readThread;
    private bool threadRunning = false;

    private readonly ILogger<PWService> _logger;

    public event EventHandler<DataEventArgs> DataReceived;

    public PipeServer(string pipeName, ILogger<PWService> logger)
    {
        _logger = logger;
        _pipeName = pipeName;
        _pipeServer = new NamedPipeServerStream(_pipeName, PipeDirection.InOut, 4, PipeTransmissionMode.Byte, PipeOptions.Asynchronous);
        _logger.LogInformation("Pipe Name: {pipeName}", pipeName);
    }

    public void Start()
    {
        Thread thread = new(async () =>
        {
            while (true)
            {
                if (_pipeServer.IsConnected)
                {
                    if (!threadRunning)
                    {
                        readThread = new Thread(ReadData)
                        {
                            Name = "NamedPipeServerReadThread",
                            IsBackground = true
                        };
                        readThread.Start();
                        threadRunning = true;
                    }
                }
                else
                {
                    if (!threadRunning)
                    {
                        _logger.LogInformation("Waiting for connecction...");
                        await _pipeServer.WaitForConnectionAsync();
                        _logger.LogInformation("Connected...");
                    }
                    else
                    {
                        _pipeServer.Dispose();
                        _pipeServer = new NamedPipeServerStream(_pipeName, PipeDirection.InOut, 4, PipeTransmissionMode.Byte, PipeOptions.Asynchronous);
                    }
                }
            }
        })
        {
            Name = "NamedPipeServerThread",
            IsBackground = true
        };

        thread.Start();
    }

    public void Stop()
    {
        readThread.Interrupt();
        _pipeServer.Close();
    }

    private async void ReadData()
    {
        byte[] buffer = new byte[1024];
        int bytesRead;

        while (true)
        {
            if (_pipeServer.IsConnected)
            {
                bytesRead = await _pipeServer.ReadAsync(buffer);

                if (bytesRead == 0)
                    break;

                string data = Encoding.UTF8.GetString(buffer, 0, bytesRead);
                string[] parts = data.Split(',');
                if (parts.Length == 2)
                {
                    DataReceived?.Invoke(this, new DataEventArgs { Title = parts[0], Message = parts[1] });
                }
            }
            else
            {
                break;
            }
        }

        threadRunning = false;
    }
    
    public async void Send(string title, string message)
    {
        try
        {
            if (_pipeServer.IsConnected)
            {
                await _pipeServer.WriteAsync(Encoding.UTF8.GetBytes(title + "," + message));
            }
        }
        catch (Exception) { }
    }
}
```

Is there any particular reason why do use Thread instead of Task? — Peter Csala
– Peter Csala, Commented Oct 4, 2023 at 7:48
@PeterCsala Well it is mostly because I like to be able to name my threads and have more control in general and I also fail to achieve the same result I get from Threads by using async Tasks — poqdavid
– poqdavid, Commented Oct 4, 2023 at 19:03

ChrisWue · Accepted Answer · 2023-10-17 03:05:39Z

Basics

The class owns an object of type NamedPipeServerStream which is IDisposable. IDisposable is a sticky interface - if you own an object that's IDisposable then so are you. Consequently your wrapper needs to be IDisposable.

Design Issues

I haven't run the code but I'm pretty sure that this path constitutes a busy wait loop that will tie up an entire CPU core when the server has an established connection from a client:

while (true)
{
    if (_pipeServer.IsConnected)
    {
        if (!threadRunning)
        {
           ....
        }
    }
    else
    {
       ...
    }

IsConnected will be true and threadRunning will be true so the whole while boils down to a repeated check on the IsConnected flag without any pause. This probably boils down to maybe 3 CPU instructions or so (2 loads and one compare) that get executed as fast as the CPU will go. This seems very wasteful.

Also be careful with Thread.Interrupt() - if the thread never blocks it will never get interrupted. In this specific case it's probably safe to assume that bytesRead = await _pipeServer.ReadAsync(buffer); will block at some point but it's not good practice. You should use CancellationTokens to cancel background work - that's what they were made for, especially in the context of async and await. Pretty much all *Async() method accept a CancellationToken.

Alternate Approach

On the whole I would probably structure the main loop something like this, wrapped in a Task that you can store as a class member:

this._cancellationTokenSource = nwe CancellationTokenSource();
this._cancellationToken = _cancellationTokenSource.Token;

var shouldQuit = false;
while (!shouldQuit)
{
    try
    {
        _pipeServer = new NamedPipeServerStream(...);
        _logger.LogInformation("Waiting for connection...");
        await _pipeServer.WaitForConnectionAsync(_cancellationToken);
        _logger.LogInformation("Connected, starting read loop...");
        await ReadDataAsync(_cancellationToken);
    }
    catch (OperationCanceledException)
    {
        _logger.LogInformation("Shutdown requested...");
        shouldQuit = true;
    }
    catch (IOException)
    {
        _Logger.LogInformation(Broken pipe, client disconnected...");
    }
}

And ReadDataAsync then boils down to something like this:

Task ReadDataAsync(CancellationToken cancellationToken)
{
    byte[] buffer = new byte[1024];
    int bytesRead;

    while (true)
    {
        bytesRead = await _pipeServer.ReadAsync(buffer, cancellationToken);

        if (bytesRead == 0)
            break;

        string data = Encoding.UTF8.GetString(buffer, 0, bytesRead);
        string[] parts = data.Split(',');
        if (parts.Length == 2)
        {
            DataReceived?.Invoke(this, new DataEventArgs { Title = parts[0], Message = parts[1] });
        }
    }
}

Taking the basics from above into account, the cleanup should happen in Dispose which should cancel the token and then await the main task to finish.

is there a way to make while loop not to consume CPU so much cause it feels like it takes like a lot — poqdavid
– poqdavid, Commented Oct 17, 2023 at 11:50
@poqdavid The alternate approach I highlighted will achieve that because it will wait for a connection (which is achieved via IO wait tasks) and the it will read data as long as some is available and the restart the loop when a problem was encountered. — ChrisWue
– ChrisWue, Commented Oct 18, 2023 at 4:07

Stack Exchange Network

Client disconnection and reconnection in C# NamedPipeServerStream

1 Answer 1

Basics

Design Issues

Alternate Approach

You must log in to answer this question.

Hot Network Questions

Client disconnection and reconnection in C# NamedPipeServerStream

1 Answer 1

Basics

Design Issues

Alternate Approach

You must log in to answer this question.

Related

Hot Network Questions