-
-
Notifications
You must be signed in to change notification settings - Fork 931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix a few issues with PipeStream #1399
Conversation
Apply similar treatment to PipeStream as sshnet#1322 did to ShellStream PipeStream now behaves much more Stream-like. In particular, it performs partial reads (instead of blocking until a certain amount of data is available), blocks until data is available (instead of returning 0 prematurely) and removes the Stream-unlike properties `BlockLastReadBuffer` and `MaxBufferLength`. Sadly I gave up trying to make a benchmark compatible with all the quirks of the previous implementation, but a dumb throughput test (reading and writing simultaneously) shows about 5.2GB/s with this implementation compared to 140MB/s previously. Some cleanup of its usage in the library followed.
// command.Result (also returned from EndExecute) consumes OutputStream, | ||
// which we've already read from, so Result will be empty. | ||
// TODO consider the suggested changes in https://github.com/sshnet/SSH.NET/issues/650 | ||
|
||
//Assert.AreEqual(expectedResult, actualResult); | ||
//Assert.AreEqual(expectedResult, command.Result); | ||
|
||
// For now just assert the current behaviour. | ||
Assert.AreEqual(0, actualResult.Length); | ||
Assert.AreEqual(0, command.Result.Length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was initially planning on getting this PR in, then getting a release out. Now I am thinking of addressing #650 beforehand by taking the break changing string Execute()
to void Execute()
in order to stop Execute
from consuming OutputStream
.
That way, we can get the CancelAsync changes (#1345), this PR with its fixes and subtle behaviour changes, and the source-breaking changes wrapped up in one release. Probably easier for consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thank you |
Thank you for fixing all these problems! Is there any estimation, in which release is the fix going to be included? |
I have a couple more fixes/changes I'd like to make to SshCommand before the next release, and then we can make one then. So I would say June, but the last time I answered that question I said May and it won't be May :-) |
Apply similar treatment to PipeStream as #1322 did to ShellStream
PipeStream now behaves much more Stream-like. In particular, it performs partial reads (instead of blocking until a certain amount of data is available), blocks until data is available (instead of returning 0 prematurely) and removes the Stream-unlike properties
BlockLastReadBuffer
andMaxBufferLength
.Sadly I gave up trying to make a benchmark compatible with all the quirks of the previous implementation, but a dumb throughput test (reading and writing simultaneously) shows about 5.2GB/s with this implementation compared to 140MB/s previously.
Some cleanup of its usage in the library followed.
contributes to #650 (does not make any of the suggested source breaking changes - still worth considering)
closes #440
closes #164
closes #12
closes #1045
closes #142
of existing PRs:
closes #1070
closes #924
closes #374
closes #13