search

sshnet / SSH.NET

SSH.NET is a Secure Shell (SSH) library for .NET, optimized for parallelism.
http://sshnet.github.io/SSH.NET/
MIT License
3.88k stars 917 forks source link

Discussion: Class-level expectSize or method-level expectSize. #1320

Closed jscarle closed 4 months ago

jscarle commented 4 months ago

@WojciechNagorski @Rob-Hague

As it stands, there are two public APIs for setting the expectSize buffer.

https://github.com/sshnet/SSH.NET/blob/d07827b6c801a91eb37d1519d56a621ed6e2b0ce/src/Renci.SshNet/SshClient.cs#L446 https://github.com/sshnet/SSH.NET/blob/d07827b6c801a91eb37d1519d56a621ed6e2b0ce/src/Renci.SshNet/SshClient.cs#L509

By default, unless specified otherwise, the expectSize will default to 2 * bufferSize. Since bufferSize defaults to 1024, that means expectSize defaults to 2048. As a default, that should cover most uses cases considering a full 80 x 25 screen is 2000 characters.

Should we remove the two public APIs that I originally added in #1207 before the next release in order to prevent any dependencies on it and allow us to rework it as a method-level parameter?

Rob-Hague commented 4 months ago

Let me get my changes up for the remaining test fixes and we can discuss it then. So far the implementation looks quite different.

jscarle commented 4 months ago

I'm adding this as a note to the conversation so I don't forget.

Implementing this at the method level would require hydration of the expect queue based on the data already contained in the incoming queue in order to produce a synchronous sliding window that parallels the existing data.

Its definately feasible, I'd just have to compare performance using benchmarks to make sure there's a gain and not a deterioration versus a class-level fixed size.

WojciechNagorski commented 4 months ago

This issue has been fixed in the 2024.0.0 version.