Problems with ShellStream

[IgorMilavec]

This initial comment will be updated with all the identified issues with ShellStream:

Defects:

1. Read(byte[], ini, int) is not blocking when no data is available (ShellStream.Read not blocking when no data available #63, Fix ShellStream.Read returning 0 prematurely #923)
2. ShellStream object is disposed when the channel is closed (We has can public _isDisposed? #424, Race Condition in ShellStream causing NullReferenceException #453, Close of channel should not dispose ShellStream #672, Cannot access a disposed object (ShellStream) #917)
3. Expect hangs if the channel is closed (Expect doesn't wake up after disconnect #670)
4. Flush() should be called automatically in Write*() (Wrong commands sequence. ShellStream.WriteLine didn't flush buffer before send its data.  #180, ShellStream: Write(String) and WriteLine(String) do not use write buffer #301)
5. ShellStream.Expect returns the entire input buffer instead of matched text (Wait for linefeed with ShellStream.Expect #140, ShellStream.Expect(Regex, TimeSpan) can return "too much" string #454, Return matched group in ShellStream.Expect #833)
6. ShellStream.Expect does not handle multibyte chars properly (ShellStream.Expect does not remove all matched data when multi-byte character encoding is used #383)

Enhancements:

1. Add Closed event (Channel_Closed in ShellStream #44, add disconnect and close events #432)
2. Recognize Lf line ending in addition to CrLf (ShellStream.ReadLine only recognises CrLf as a line ending #748)
3. Timeouts are not observed properly in Expect and ReadLine ([Feature Request] ShellStream.Expect - Abort if No Match Found Within a Specified Timeout Period #365, ShellStream.Expect TimeSpan timeout, not working. #714)
4. Flush() should be called automatically in Read() and Write*() (ShellStream: Buffer is not flushed before read #302, Expect reads and returns data out of order #444)
5. Write(string and WriteLine(string) do not use write buffer (ShellStream: Write(String) and WriteLine(String) do not use write buffer #301)
6. Add ability to change window size (Terminal window resizing #40)
7. Inefficient buffer implementation (Significantly improved performance and fixed bug with ShellStream's Expect methods. #793, Optimized incoming bytes queue. Release lock asap. #800)

Requested examples:

• How to run a background process (&) (Launch background job #16)

[IgorMilavec]

I went through all the open issues and tried to summarize all of the problems with ShellStream. I you have something to add or discuss, please comment below and I will edit the first comment so we have a summarized description.

[IgorMilavec]

Added #432, #793 and #800.

[PatrickSchmidtSE]

Thank you for that :)

[IgorMilavec]

OK, so obviously we have a lot to do with ShellStream... IMO bugs should be fixed ASAP, but there is a large number of them, so fixing them one at a time might take a long time. Due to partial interdependence of some of the bugs, handling them in separate PRs would also create merge conflicts. @drieseng I know you prefer small focused PRs, but would you be willing to accept batched fixes in this case?

As tests did not catch these defects, we should first add a (failing) test for each of these scenarios before fixing them.

As a general design note it seems to me that a lot of problems originate from the ShellStream providing too much functionality. A lot of functionality is duplicated with StreamReader (which will properly handle character encoding). Expect() seems to be a nice utility for simple scenarios, but fails or is inefficient in more complex scenarios. I know I'm discussing breaking changes here, so we should thread carefully, but I really feel that with proper class design SSH.Net could be more stable and more performant. And we don't need to do breaking changes over night, [Obsolete] is a great tool in the long run.

[drieseng]

@IgorMilavec I propose to with an issue or discussion that we'll use to identify the bugs and any proposed changes.
We can then see if we group multiple fixes or changes in a single PR.
Works for you?

Update:
Perhaps I should read the description of the issue :-) LOL

[drieseng]

@IgorMilavec I'd start with fixing the first defect. I'd also like to add some integration tests for ShellStream (to https://github.com/sshnet/IntegrationTests).

Let's also discuss what we want to do for the second defect. I see the following scenario's:

• the server closes the channel (shell)
• the server closes the SSH session.
• the client closes the channel (shell). eg. disposes the ShellStream.
• the client closes the SSH session (eg. disposes the SshClient).

Do we want to expose an event? Should the event allow one to distinguish the exact scenario?
Should the event also fire if the client (user) closes the channel or SSH session?

[VAllens]

That's great.

[IgorMilavec]

I've created #923 for the first defect.

Here is the behavior I would expect in these cases:

• the server closes the channel (shell): Read returns 0, Write returns 0
• the server closes the SSH session: Same as above
• the client closes the channel (shell). eg. disposes the ShellStream.: SImple: ObjectDisposedException
• the client closes the SSH session (eg. disposes the SshClient): this is the tricky one... the same behavior one gets when creating a NetworkStream from a Socket and then closes the socket?

So, generally, I would like ShellStream to behave the way dotnet built-in streams behave and the way every developer intuitively expects it to behave.

As for the events: at this time I'd rather not add any new API surface, unless someone shows he needs them and there is no other way to do it.

[WojciechNagorski]

@IgorMilavec Do you want to continue your work on ShellStream? I intend to include all ShellStream related tasks in the next release.

FYI @Rob-Hague

[jscarle]

@WojciechNagorski There appears to be only 4 issues left that are linked to this discussion.

• I think Launch background job #16 can now be considered a stale discussion. It mostly seems to be a user discussion and considering this was 8 years ago, I think it's safe to close this issue.
• Terminal window resizing #40 seems like a reasonable request, but I believe this should addressed in a future release.
• As for Channel_Closed in ShellStream #44, I need your opinion or @Rob-Hague 's opinion. I don't know what the current thoughts or plans are in terms of events. I know that .NET in general seems to be moving away from events and replacing them with Action and Func, so I'm not sure if this should be planned or not.
• I'm not sure we'd want to do We has can public _isDisposed? #424. IsDisposed seems to mostly exist in concepts related to the UI or direct ties to unmanaged ressources. So again, is this something we still want to do?

These issues are generally very old and haven't really generated repeated discussions or issues. Most of these may no longer apply with all of the recent internal changes.

My proposal would be to close #16, #44, #424 and keep only #40 open but planned for a future release.

[WojciechNagorski]

I keep open only #40
@jscarle Thanks for your summary!

[scott-xu]

I think #40 would get fixed once #1062 is ready and merged. @glenkleidon