H.Pipes icon indicating copy to clipboard operation
H.Pipes copied to clipboard

Implementation questions

Open alexeygritsenko opened this issue 1 year ago • 1 comments

Hi, thanks for the great job. I have noticed several problems

  1. After approximately 7,000 clients contacted, 4GB of memory disappears in the system. My solution:
_server.ClientDisconnected += async (o, args) =>
{
    await args.Connection.DisposeAsync();
    Debug.WriteLine($"Client {args.Connection.PipeName} disconnected");
};

After that the memory ceased to disappear. This helped free up memory.

  1. After some time, the server stops responding. However, it does not log an error anywhere. To reproduce this quickly, you need to terminate the client in the middle of the connection. Thus the exception is returned from the handshakeWrapper

https://github.com/HavenDV/H.Pipes/blob/0db095207ddfabfabb4a2c74f34ece8c26962e0b/src/libs/H.Pipes/PipeServer.cs#LL182C33-L182C33

Then the break stops the while cycle. New clients can no longer connect to the server.

https://github.com/HavenDV/H.Pipes/blob/0db095207ddfabfabb4a2c74f34ece8c26962e0b/src/libs/H.Pipes/PipeServer.cs#LL195C25-L195C25

My solution: replace break; to throw; Question: How reasonable was the break in this place? Is my fix correct or should this cycle be created per client?

  1. I looked at this commit https://github.com/HavenDV/H.Pipes/commit/0db095207ddfabfabb4a2c74f34ece8c26962e0b here I have doubts about the correct usage System.Memory. I'd rather not fill head with newfangled classes and conditions, instead add a warning to Suppress CA1835 message. And then use a single codebase, like this
    {
        var buffer = new byte[length];
        var read = await BaseStream.ReadAsync(buffer, 0, buffer.Length, cancellationToken).ConfigureAwait(false);

        if (read != length)
        {
            return throwIfReadLessThanLength
                ? throw new IOException($"Expected {length} bytes but read {read}")
                : Array.Empty<byte>();
        }

        return buffer;
    }

this code looks cleaner and easier to maintain. Same with WriteAsync.

alexeygritsenko avatar Jun 13 '23 14:06 alexeygritsenko