Skip to content

Commit c70d95b

Browse files
Rob-HagueRob-Hague
committed
Fix a few issues with PipeStream
Apply similar treatment to PipeStream as sshnet#1322 did to ShellStream PipeStream will now behave 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`. It should also be a fair bit faster, but sadly I gave up trying to make a benchmark compatible with all the quirks of the previous implementation.
1 parent 1de33c4 commit c70d95b

16 files changed

+330
-684
lines changed

‎src/Renci.SshNet/Common/PipeStream.cs‎

Lines changed: 118 additions & 295 deletions
Large diffs are not rendered by default.

‎src/Renci.SshNet/ScpClient.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,7 @@ public void Upload(Stream source, string path)
234234
using (var channel = Session.CreateChannelSession())
235235
{
236236
channel.DataReceived += (sender, e) => input.Write(e.Data, 0, e.Data.Length);
237+
channel.Closed += (sender, e) => input.Dispose();
237238
channel.Open();
238239

239240
// Pass only the directory part of the path to the server, and use the (hidden) -d option to signal
@@ -273,6 +274,7 @@ public void Upload(FileInfo fileInfo, string path)
273274
using (var channel = Session.CreateChannelSession())
274275
{
275276
channel.DataReceived += (sender, e) => input.Write(e.Data, 0, e.Data.Length);
277+
channel.Closed += (sender, e) => input.Dispose();
276278
channel.Open();
277279

278280
// Pass only the directory part of the path to the server, and use the (hidden) -d option to signal
@@ -324,6 +326,7 @@ public void Upload(DirectoryInfo directoryInfo, string path)
324326
using (var channel = Session.CreateChannelSession())
325327
{
326328
channel.DataReceived += (sender, e) => input.Write(e.Data, 0, e.Data.Length);
329+
channel.Closed += (sender, e) => input.Dispose();
327330
channel.Open();
328331

329332
// start copy with the following options:
@@ -367,6 +370,7 @@ public void Download(string filename, FileInfo fileInfo)
367370
using (var channel = Session.CreateChannelSession())
368371
{
369372
channel.DataReceived += (sender, e) => input.Write(e.Data, 0, e.Data.Length);
373+
channel.Closed += (sender, e) => input.Dispose();
370374
channel.Open();
371375

372376
// Send channel command request
@@ -407,6 +411,7 @@ public void Download(string directoryName, DirectoryInfo directoryInfo)
407411
using (var channel = Session.CreateChannelSession())
408412
{
409413
channel.DataReceived += (sender, e) => input.Write(e.Data, 0, e.Data.Length);
414+
channel.Closed += (sender, e) => input.Dispose();
410415
channel.Open();
411416

412417
// Send channel command request
@@ -447,6 +452,7 @@ public void Download(string filename, Stream destination)
447452
using (var channel = Session.CreateChannelSession())
448453
{
449454
channel.DataReceived += (sender, e) => input.Write(e.Data, 0, e.Data.Length);
455+
channel.Closed += (sender, e) => input.Dispose();
450456
channel.Open();
451457

452458
// Send channel command request

‎src/Renci.SshNet/ShellStream.cs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,7 @@ public override bool CanWrite
182182
get { return !_disposed; }
183183
}
184184

185-
/// <summary>
186-
/// This method does nothing.
187-
/// </summary>
185+
/// <inheritdoc/>
188186
public override void Flush()
189187
{
190188
ThrowIfDisposed();

‎src/Renci.SshNet/SshCommand.cs‎

Lines changed: 47 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ public class SshCommand : IDisposable
2828
private EventWaitHandle _sessionErrorOccuredWaitHandle;
2929
private EventWaitHandle _commandCancelledWaitHandle;
3030
private Exception _exception;
31-
private StringBuilder _result;
32-
private StringBuilder _error;
31+
private string _result;
32+
private string _error;
3333
private bool _hasError;
3434
private bool _isDisposed;
3535
private bool _isCancelled;
@@ -109,21 +109,22 @@ public string Result
109109
{
110110
get
111111
{
112-
_result ??= new StringBuilder();
112+
if (_result is not null)
113+
{
114+
return _result;
115+
}
113116

114-
if (OutputStream != null && OutputStream.Length > 0)
117+
if (OutputStream is null)
115118
{
116-
using (var sr = new StreamReader(OutputStream,
117-
_encoding,
118-
detectEncodingFromByteOrderMarks: true,
119-
bufferSize: 1024,
120-
leaveOpen: true))
121-
{
122-
_ = _result.Append(sr.ReadToEnd());
123-
}
119+
return string.Empty;
124120
}
125121

126-
return _result.ToString();
122+
using (var sr = new StreamReader(OutputStream,
123+
_encoding,
124+
detectEncodingFromByteOrderMarks: true))
125+
{
126+
return _result = sr.ReadToEnd();
127+
}
127128
}
128129
}
129130

@@ -134,26 +135,22 @@ public string Error
134135
{
135136
get
136137
{
137-
if (_hasError)
138+
if (_error is not null)
139+
{
140+
return _error;
141+
}
142+
143+
if (ExtendedOutputStream is null || !_hasError)
138144
{
139-
_error ??= new StringBuilder();
140-
141-
if (ExtendedOutputStream != null && ExtendedOutputStream.Length > 0)
142-
{
143-
using (var sr = new StreamReader(ExtendedOutputStream,
144-
_encoding,
145-
detectEncodingFromByteOrderMarks: true,
146-
bufferSize: 1024,
147-
leaveOpen: true))
148-
{
149-
_ = _error.Append(sr.ReadToEnd());
150-
}
151-
}
152-
153-
return _error.ToString();
145+
return string.Empty;
154146
}
155147

156-
return string.Empty;
148+
using (var sr = new StreamReader(ExtendedOutputStream,
149+
_encoding,
150+
detectEncodingFromByteOrderMarks: true))
151+
{
152+
return _error = sr.ReadToEnd();
153+
}
157154
}
158155
}
159156

@@ -265,26 +262,16 @@ public IAsyncResult BeginExecute(AsyncCallback callback, object state)
265262
throw new ArgumentException("CommandText property is empty.");
266263
}
267264

268-
var outputStream = OutputStream;
269-
if (outputStream is not null)
270-
{
271-
outputStream.Dispose();
272-
OutputStream = null;
273-
}
274-
275-
var extendedOutputStream = ExtendedOutputStream;
276-
if (extendedOutputStream is not null)
277-
{
278-
extendedOutputStream.Dispose();
279-
ExtendedOutputStream = null;
280-
}
265+
OutputStream?.Dispose();
266+
ExtendedOutputStream?.Dispose();
281267

282268
// Initialize output streams
283269
OutputStream = new PipeStream();
284270
ExtendedOutputStream = new PipeStream();
285271

286272
_result = null;
287273
_error = null;
274+
_hasError = false;
288275
_callback = callback;
289276

290277
_channel = CreateChannel();
@@ -341,13 +328,21 @@ public string EndExecute(IAsyncResult asyncResult)
341328

342329
_inputStream?.Close();
343330

344-
// wait for operation to complete (or time out)
345-
WaitOnHandle(_asyncResult.AsyncWaitHandle);
331+
try
332+
{
333+
// wait for operation to complete (or time out)
334+
WaitOnHandle(_asyncResult.AsyncWaitHandle);
335+
}
336+
finally
337+
{
338+
UnsubscribeFromEventsAndDisposeChannel(_channel);
339+
_channel = null;
346340

347-
UnsubscribeFromEventsAndDisposeChannel(_channel);
348-
_channel = null;
341+
OutputStream?.Dispose();
342+
ExtendedOutputStream?.Dispose();
349343

350-
commandAsyncResult.EndCalled = true;
344+
commandAsyncResult.EndCalled = true;
345+
}
351346

352347
if (!_isCancelled)
353348
{
@@ -437,8 +432,8 @@ private void Session_ErrorOccured(object sender, ExceptionEventArgs e)
437432

438433
private void SetAsyncComplete()
439434
{
440-
OutputStream?.Flush();
441-
ExtendedOutputStream?.Flush();
435+
OutputStream?.Dispose();
436+
ExtendedOutputStream?.Dispose();
442437

443438
_asyncResult.IsCompleted = true;
444439

@@ -480,11 +475,7 @@ private void Channel_RequestReceived(object sender, ChannelRequestEventArgs e)
480475

481476
private void Channel_ExtendedDataReceived(object sender, ChannelExtendedDataEventArgs e)
482477
{
483-
if (ExtendedOutputStream != null)
484-
{
485-
ExtendedOutputStream.Write(e.Data, 0, e.Data.Length);
486-
ExtendedOutputStream.Flush();
487-
}
478+
ExtendedOutputStream?.Write(e.Data, 0, e.Data.Length);
488479

489480
if (e.DataTypeCode == 1)
490481
{
@@ -494,11 +485,7 @@ private void Channel_ExtendedDataReceived(object sender, ChannelExtendedDataEven
494485

495486
private void Channel_DataReceived(object sender, ChannelDataEventArgs e)
496487
{
497-
if (OutputStream != null)
498-
{
499-
OutputStream.Write(e.Data, 0, e.Data.Length);
500-
OutputStream.Flush();
501-
}
488+
OutputStream?.Write(e.Data, 0, e.Data.Length);
502489

503490
if (_asyncResult != null)
504491
{

‎test/Renci.SshNet.IntegrationBenchmarks/SshClientBenchmark.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,18 @@ public string RunCommand()
7272
return _sshClient!.RunCommand("echo $'test !@#$%^&*()_+{}:,./<>[];\\|'").Result;
7373
}
7474

75+
[Benchmark]
76+
public string RunBigCommand()
77+
{
78+
using var command = _sshClient!.CreateCommand("head -c 10000000 /dev/urandom | base64"); // 10MB of data please
79+
80+
var asyncResult = command.BeginExecute();
81+
82+
command.OutputStream.CopyTo(Stream.Null);
83+
84+
return command.EndExecute(asyncResult);
85+
}
86+
7587
[Benchmark]
7688
public string ShellStreamReadLine()
7789
{

‎test/Renci.SshNet.IntegrationTests/SshClientTests.cs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,28 @@ public void Echo_Command_with_all_characters()
2222
Assert.AreEqual("test !@#$%^&*()_+{}:,./<>[];\\|\n", response.Result);
2323
}
2424

25+
[TestMethod]
26+
public void Test_BigCommand()
27+
{
28+
using var command = _sshClient.CreateCommand("head -c 10000000 /dev/urandom | base64"); // 10MB of data please
29+
30+
var asyncResult = command.BeginExecute();
31+
32+
long totalBytesRead = 0;
33+
int bytesRead;
34+
byte[] buffer = new byte[4096];
35+
36+
while ((bytesRead = command.OutputStream.Read(buffer, 0, buffer.Length)) != 0)
37+
{
38+
totalBytesRead += bytesRead;
39+
}
40+
41+
var result = command.EndExecute(asyncResult);
42+
43+
Assert.AreEqual(13_508_775, totalBytesRead);
44+
Assert.AreEqual(0, result.Length);
45+
}
46+
2547
[TestMethod]
2648
public void Send_InputStream_to_Command()
2749
{

‎test/Renci.SshNet.IntegrationTests/SshTests.cs‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -229,15 +229,7 @@ public void Ssh_Command_IntermittendOutput_EndExecute()
229229
}
230230
}
231231

232-
/// <summary>
233-
/// Ignored for now, because:
234-
/// * OutputStream.Read(...) does not block when no data is available
235-
/// * SshCommand.(Begin)Execute consumes *OutputStream*, advancing its position.
236-
///
237-
/// https://github.com/sshnet/SSH.NET/issues/650
238-
/// </summary>
239232
[TestMethod]
240-
[Ignore]
241233
public void Ssh_Command_IntermittendOutput_OutputStream()
242234
{
243235
const string remoteFile = "/home/sshnet/test.sh";
@@ -297,8 +289,16 @@ public void Ssh_Command_IntermittendOutput_OutputStream()
297289

298290
var actualResult = command.EndExecute(asyncResult);
299291

300-
Assert.AreEqual(expectedResult, actualResult);
301-
Assert.AreEqual(expectedResult, command.Result);
292+
// command.Result (also returned from EndExecute) consumes OutputStream,
293+
// which we've already read from, so Result will be empty.
294+
// TODO consider the suggested changes in https://github.com/sshnet/SSH.NET/issues/650
295+
296+
//Assert.AreEqual(expectedResult, actualResult);
297+
//Assert.AreEqual(expectedResult, command.Result);
298+
299+
// For now just assert the current behaviour.
300+
Assert.AreEqual(0, actualResult.Length);
301+
Assert.AreEqual(0, command.Result.Length);
302302
}
303303
}
304304
finally

‎test/Renci.SshNet.Tests/.editorconfig‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,10 @@ dotnet_diagnostic.MA0110.severity = silent
309309
# https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0026.md
310310
dotnet_diagnostic.MA0026.severity = silent
311311

312+
# MA0042: Do not use blocking calls in an async method
313+
# https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0042.md
314+
dotnet_diagnostic.MA0042.severity = silent
315+
312316
#### .NET Compiler Platform analysers rules ####
313317

314318
# CA1031: Do not catch general exception types
@@ -401,4 +405,8 @@ dotnet_diagnostic.IDE0032.severity = silent
401405

402406
# CA1812: Avoid uninstantiated internal classes
403407
# https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1812
404-
dotnet_diagnostic.CA1812.severity = silent
408+
dotnet_diagnostic.CA1812.severity = silent
409+
410+
# CA1849: Call async methods when in an async method
411+
# https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/CA1849
412+
dotnet_diagnostic.CA1849.severity = silent

0 commit comments

Comments
 (0)