Implementation questions

[alexeygritsenko]

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.

2. 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?

3. I looked at this commit 0db0952 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.

[HavenDV]

Hello. Many thanks for the detailed examples and comments.

1. The code was written quite a while ago and I haven't actively tested it in an environment with thousands of connections. But it's strange that I made such an obvious mistake. Definitely need to fix this.
2. Yes, I agree with you in the proposed solution
3. I think that #if constructs will have to be dealt with anyway, they speed up the code quite a lot for net7.0. So I don't see the point in getting rid of it here. As for the use of Memory - yes, it looks a bit strange, but the library has enough data transfer tests and they all pass in net7.0, so I think everything is correct.

I will be glad to accept your PR on the first and second points, or I will do it myself in the future. Thank you!