High memory allocation using SshCommand

[Levi--G]

Hi,

I noticed using SSH.Net there are a few issues using SshCommand:

• Allocation of 2kB of buffer for every command, even short ones without output and without monitoring output
• Not reading output causes these buffers to grow indefinitely for long running commands
• No way to discard this output easily
• Using sshcommands many at a time in a pool causes huge GC pressure with the many large allocations

An example from a single command running without cleaning the streams shows it allocating over 300kB of memory in a VS snapshot.

Possible solutions:

• Allocate the pipestreams only when accessed, discarding input otherwise (breaking change)
• Creating a new sshcommand that does not allocate.

I will be creating a pull request for scenario 2 as i need a solution for a project asap.

Thanks for the library!

[Levi--G]

Forgot to include the sample:

SshClient client = new SshClient(...);
client.Connect();
var c = client.RunCommand("for i in {1..1000} ; do echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ; done");
//300kB allocated

[Rob-Hague]

The latest develop branch has removed quite a few allocations, so you can try that. But yes, command output is buffered if not observed.

If you don't care about the output, you could do something like

Task drainOutput = cmd.OutputStream.CopyToAsync(Stream.Null);

Creating a new sshcommand that does not allocate.
I will be creating a pull request for scenario 2 as i need a solution for a project asap.

Adding another new type for a command is unlikely to be accepted. It might be better to at least flesh out a proposal if you are still convinced. I am just trying to save you from doing a lot of work that might not lead anywhere. Of course, you can do what you like in your own fork.

[Levi--G]

The latest develop branch has removed quite a few allocations, so you can try that. But yes, command output is buffered if not observed.

The example with 2kB and 300kB allocations are from the develop branch.

If you don't care about the output, you could do something like

Task drainOutput = cmd.OutputStream.CopyToAsync(Stream.Null);

This causes a thread to needlessly spin and consume the data, quite wasteful as well in my opinion.

Adding another new type for a command is unlikely to be accepted. It might be better to at least flesh out a proposal if you are still convinced. I am just trying to save you from doing a lot of work that might not lead anywhere. Of course, you can do what you like in your own fork.

Could be that it won't be accepted, i don't mind since i need it anyways, easier to have to update a fork with one class extra than having to make a new library from scratch. Current class just doesn't work for my scenario anyways. I am creating and almost done with my example of how it could be made, i'll submit it as pull request as a proposal and why i chose the approach i did, up to you to accept it or not. No hard feelings. 👍

[Rob-Hague]

(Keeping the discussion here to not split it in two places)

Yeah so as discussed, won't be taking #1778. But it seems plausible to make the setters of OutputStream and ExtendedOutputStream public rather than private, with some warnings about multi-threaded access and some disabling of the re-initialisation in ExecuteAsync. Would that solve things for you?

[Levi--G]

(Keeping the discussion here to not split it in two places)

Yeah so as discussed, won't be taking #1778. But it seems plausible to make the setters of OutputStream and ExtendedOutputStream public rather than private, with some warnings about multi-threaded access and some disabling of the re-initialisation in ExecuteAsync. Would that solve things for you?

Depending on how it is done, no, since the streams are initialized even if we replace them afterward it will still have been allocated causing gc pressure, and not to mention further complicating matters for simple commands that we don't want to read output from. As i have already show in the the PR the current api unless it gets a breaking change cannot be modified in a way that doesn't do the large allocations.

What would you propose to change the allocation then? Which is what this issue is about.

One way forward could be to make the streams only allocate on usage but then either we have one of the creation methods have a breaking change or we would need to add a new one anyways, which would make it confusing for new users.

[Rob-Hague]

I think you are talking about the 2kB allocation of PipeStream buffers, rather than the 300kB of command output that could feasibly be removed by allowing e.g. cmd.OutputStream = Stream.Null;. In which case I have to wonder how large % of your memory profile do such buffers represent. I am all for reducing allocations, but I think there are lower-hanging fruit in the library.

[Rob-Hague]

if it really does show up, it is probably possible to init the PipeStream with a small buffer and the logic should already take care of scaling up. it would just penalise other use cases

[Levi--G]

if it really does show up, it is probably possible to init the PipeStream with a small buffer and the logic should already take care of scaling up. it would just penalise other use cases

Yes, that is why i didn't suggest a breaking change since that would impact all existing scenarios.
Regarding the 2kB yes that is atm a real issue, the whole reason i started this topic is because a low memory device was executing a lot of commands (100s) at the same time and it ran out and started lagging due to the GC not keeping up until the app was killed. I Agree there are as you call them "lower-hanging fruit". But this is one I am prepared to put in the work to find a solution since i'm actively having an issue atm. :)

Well at the moment i solved it with my fork so it is not urgent, but if we can find some common ground on what change to make i don't mind making it.

What about these changes to sshcommand:
-add the events for direct usage to also fix the other open issue
-add a bool option to the constructor to preallocate the buffers or not and make it default true
-make the buffers nullable and assignable and only fill them when not null
-upon accessing the buffer property create the buffers and start filling them from that point on (similar to the input stream?)
-make the run method use the preallocation so all output is logged (no change)
-make the create method not use the allocation (minor breaking change?)

would that be acceptible?

[Rob-Hague]

Alright, I'd be happy to go with lazy init and public setters. Something like:

private Stream? _outputStream;

// specifies that the SshCommand object is not the owner of the stream and thus disables re-init and disposal.
private bool _outputStreamHasBeenSetExternally;

public Stream OutputStream
{
    get => _outputStream ??= new PipeStream();
    set
    {
        ArgumentNullException.ThrowIfNull(value);
        _outputStreamHasBeenSetExternally = true;
        _outputStream = value;
    }
}

then you should be able to do

using SshCommand cmd = sshClient.CreateCommand(...);
cmd.OutputStream = Stream.Null;
cmd.ExtendedOutputStream = Stream.Null;

I think that should do it.