Skip to content

Commit 5408aac

Browse files
committed
Avoid reading from the server twice to determine EOF.
When a server returns less number of bytes than the read buffer size, this *may* indicate that EOF has been reached. A subsequent (SSH_FXP_READ) server request is necessary to make sure EOF has effectively been reached. Breaking out of the read loop avoids reading from the server twice to determine EOF: once in the read loop, and once upon the next Read or ReadByte invocation.
1 parent 416f7be commit 5408aac

6 files changed

+233
-28
lines changed
Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
namespace Renci.SshNet.Tests.Classes.Sftp
99
{
1010
[TestClass]
11-
public class SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCount : SftpFileStreamTestBase
11+
public class SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCountAndEqualToBufferSize : SftpFileStreamTestBase
1212
{
1313
private string _path;
1414
private SftpFileStream _target;
@@ -35,12 +35,14 @@ protected override void SetupData()
3535
_readBufferSize = 20;
3636
_writeBufferSize = 500;
3737

38-
_numberOfBytesToRead = 20;
38+
_numberOfBytesToRead = (int) _readBufferSize + 5; // greather than read buffer size
3939
_buffer = new byte[_numberOfBytesToRead];
40-
_serverData1Length = _numberOfBytesToRead - 5;
40+
_serverData1Length = (int) _readBufferSize; // equal to read buffer size
4141
_serverData1 = GenerateRandom(_serverData1Length, random);
42-
_serverData2Length = _numberOfBytesToRead - _serverData1Length + 3;
42+
_serverData2Length = (int) _readBufferSize; // equal to read buffer size
4343
_serverData2 = GenerateRandom(_serverData2Length, random);
44+
45+
Assert.IsTrue(_serverData1Length < _numberOfBytesToRead && _serverData1Length == _readBufferSize);
4446
}
4547

4648
protected override void SetupMocks()
@@ -61,7 +63,7 @@ protected override void SetupMocks()
6163
.Setup(p => p.RequestRead(_handle, 0UL, _readBufferSize))
6264
.Returns(_serverData1);
6365
SftpSessionMock.InSequence(MockSequence)
64-
.Setup(p => p.RequestRead(_handle, (ulong) _serverData1.Length, _readBufferSize))
66+
.Setup(p => p.RequestRead(_handle, (ulong)_serverData1.Length, _readBufferSize))
6567
.Returns(_serverData2);
6668
}
6769

@@ -91,7 +93,7 @@ protected override void Act()
9193
[TestMethod]
9294
public void ReadShouldHaveReturnedTheNumberOfBytesRequested()
9395
{
94-
Assert.AreEqual(_buffer.Length, _actual);
96+
Assert.AreEqual(_numberOfBytesToRead, _actual);
9597
}
9698

9799
[TestMethod]
@@ -104,11 +106,11 @@ public void ReadShouldHaveWrittenBytesToTheCallerSuppliedBuffer()
104106
}
105107

106108
[TestMethod]
107-
public void PositionShouldReturnNumberOfBytesWrittenToBuffer()
109+
public void PositionShouldReturnNumberOfBytesWrittenToCallerProvidedBuffer()
108110
{
109111
SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
110112

111-
Assert.AreEqual(_buffer.Length, _target.Position);
113+
Assert.AreEqual(_actual, _target.Position);
112114

113115
SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
114116
}
@@ -134,7 +136,7 @@ public void ReadShouldReturnAllRemaningBytesFromReadBufferWhenCountIsEqualToNumb
134136
public void ReadShouldReturnAllRemaningBytesFromReadBufferAndReadAgainWhenCountIsGreaterThanNumberOfRemainingBytesAndNewReadReturnsZeroBytes()
135137
{
136138
SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
137-
SftpSessionMock.InSequence(MockSequence).Setup(p => p.RequestRead(_handle, (ulong) (_serverData1Length + _serverData2Length), _readBufferSize)).Returns(Array<byte>.Empty);
139+
SftpSessionMock.InSequence(MockSequence).Setup(p => p.RequestRead(_handle, (ulong)(_serverData1Length + _serverData2Length), _readBufferSize)).Returns(Array<byte>.Empty);
138140

139141
var numberOfBytesRemainingInReadBuffer = _serverData1Length + _serverData2Length - _numberOfBytesToRead;
140142

@@ -147,7 +149,7 @@ public void ReadShouldReturnAllRemaningBytesFromReadBufferAndReadAgainWhenCountI
147149
Assert.AreEqual(0, _buffer[numberOfBytesRemainingInReadBuffer]);
148150

149151
SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
150-
SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong) (_serverData1Length + _serverData2Length), _readBufferSize));
152+
SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong)(_serverData1Length + _serverData2Length), _readBufferSize));
151153
}
152154
}
153155
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
using System;
2+
using System.IO;
3+
using Microsoft.VisualStudio.TestTools.UnitTesting;
4+
using Moq;
5+
using Renci.SshNet.Common;
6+
using Renci.SshNet.Sftp;
7+
using Renci.SshNet.Tests.Common;
8+
9+
namespace Renci.SshNet.Tests.Classes.Sftp
10+
{
11+
[TestClass]
12+
public class SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCountAndLessThanBufferSize : SftpFileStreamTestBase
13+
{
14+
private string _path;
15+
private SftpFileStream _target;
16+
private byte[] _handle;
17+
private uint _bufferSize;
18+
private uint _readBufferSize;
19+
private uint _writeBufferSize;
20+
private int _actual;
21+
private byte[] _buffer;
22+
private byte[] _serverData;
23+
private int _serverDataLength;
24+
private int _numberOfBytesToRead;
25+
private byte[] _originalBuffer;
26+
27+
protected override void SetupData()
28+
{
29+
base.SetupData();
30+
31+
var random = new Random();
32+
_path = random.Next().ToString();
33+
_handle = GenerateRandom(5, random);
34+
_bufferSize = (uint)random.Next(1, 1000);
35+
_readBufferSize = 20;
36+
_writeBufferSize = 500;
37+
38+
_numberOfBytesToRead = (int) _readBufferSize + 2; // greater than read buffer size
39+
_originalBuffer = GenerateRandom(_numberOfBytesToRead, random);
40+
_buffer = _originalBuffer.Copy();
41+
42+
_serverDataLength = (int) _readBufferSize - 1; // less than read buffer size
43+
_serverData = GenerateRandom(_serverDataLength, random);
44+
45+
Assert.IsTrue(_serverDataLength < _numberOfBytesToRead && _serverDataLength < _readBufferSize);
46+
}
47+
48+
protected override void SetupMocks()
49+
{
50+
SftpSessionMock.InSequence(MockSequence)
51+
.Setup(p => p.RequestOpen(_path, Flags.Read, false))
52+
.Returns(_handle);
53+
SftpSessionMock.InSequence(MockSequence)
54+
.Setup(p => p.CalculateOptimalReadLength(_bufferSize))
55+
.Returns(_readBufferSize);
56+
SftpSessionMock.InSequence(MockSequence)
57+
.Setup(p => p.CalculateOptimalWriteLength(_bufferSize, _handle))
58+
.Returns(_writeBufferSize);
59+
SftpSessionMock.InSequence(MockSequence)
60+
.Setup(p => p.IsOpen)
61+
.Returns(true);
62+
SftpSessionMock.InSequence(MockSequence)
63+
.Setup(p => p.RequestRead(_handle, 0UL, _readBufferSize))
64+
.Returns(_serverData);
65+
}
66+
67+
[TestCleanup]
68+
public void TearDown()
69+
{
70+
SftpSessionMock.InSequence(MockSequence)
71+
.Setup(p => p.RequestClose(_handle));
72+
}
73+
74+
protected override void Arrange()
75+
{
76+
base.Arrange();
77+
78+
_target = new SftpFileStream(SftpSessionMock.Object,
79+
_path,
80+
FileMode.Open,
81+
FileAccess.Read,
82+
(int)_bufferSize);
83+
}
84+
85+
protected override void Act()
86+
{
87+
_actual = _target.Read(_buffer, 0, _numberOfBytesToRead);
88+
}
89+
90+
[TestMethod]
91+
public void ReadShouldHaveReturnedTheNumberOfBytesReturnedByTheReadFromTheServer()
92+
{
93+
Assert.AreEqual(_serverDataLength, _actual);
94+
}
95+
96+
[TestMethod]
97+
public void ReadShouldHaveWrittenBytesToTheCallerSuppliedBufferAndRemainingBytesShouldRemainUntouched()
98+
{
99+
Assert.IsTrue(_serverData.IsEqualTo(_buffer.Take(_serverDataLength)));
100+
Assert.IsTrue(_originalBuffer.Take(_serverDataLength, _originalBuffer.Length - _serverDataLength).IsEqualTo(_buffer.Take(_serverDataLength, _buffer.Length - _serverDataLength)));
101+
}
102+
103+
[TestMethod]
104+
public void PositionShouldReturnNumberOfBytesWrittenToCallerProvidedBuffer()
105+
{
106+
SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
107+
108+
Assert.AreEqual(_actual, _target.Position);
109+
110+
SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
111+
}
112+
113+
[TestMethod]
114+
public void SubsequentReadShouldReadAgainFromCurrentPositionFromServerAndReturnZeroWhenServerReturnsZeroBytes()
115+
{
116+
SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
117+
SftpSessionMock.InSequence(MockSequence)
118+
.Setup(p => p.RequestRead(_handle, (ulong) _actual, _readBufferSize))
119+
.Returns(Array<byte>.Empty);
120+
121+
var buffer = _originalBuffer.Copy();
122+
var actual = _target.Read(buffer, 0, buffer.Length);
123+
124+
Assert.AreEqual(0, actual);
125+
Assert.IsTrue(_originalBuffer.IsEqualTo(buffer));
126+
127+
SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong)_actual, _readBufferSize), Times.Once);
128+
SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
129+
}
130+
131+
[TestMethod]
132+
public void SubsequentReadShouldReadAgainFromCurrentPositionFromServerAndNotUpdatePositionWhenServerReturnsZeroBytes()
133+
{
134+
SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
135+
SftpSessionMock.InSequence(MockSequence)
136+
.Setup(p => p.RequestRead(_handle, (ulong)_actual, _readBufferSize))
137+
.Returns(Array<byte>.Empty);
138+
SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
139+
140+
_target.Read(new byte[10], 0, 10);
141+
142+
Assert.AreEqual(_actual, _target.Position);
143+
144+
SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong)_actual, _readBufferSize), Times.Once);
145+
SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(3));
146+
}
147+
}
148+
}

src/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadMoreBytesFromServerThanCount.cs

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using Microsoft.VisualStudio.TestTools.UnitTesting;
44
using Moq;
55
using Renci.SshNet.Sftp;
6+
using Renci.SshNet.Common;
67

78
namespace Renci.SshNet.Tests.Classes.Sftp
89
{
@@ -81,40 +82,58 @@ protected override void Act()
8182
}
8283

8384
[TestMethod]
84-
public void ReadShouldHaveReturnedTheNumberOfBytesRequested()
85+
public void ReadShouldHaveReturnedTheNumberOfBytesWrittenToCallerSuppliedBuffer()
8586
{
8687
Assert.AreEqual(_numberOfBytesToRead, _actual);
8788
}
8889

8990
[TestMethod]
9091
public void ReadShouldHaveWrittenBytesToTheCallerSuppliedBuffer()
9192
{
92-
Assert.IsTrue(_serverData.Take(_buffer.Length).IsEqualTo(_buffer));
93+
Assert.IsTrue(_serverData.Take(_actual).IsEqualTo(_buffer));
9394
}
9495

9596
[TestMethod]
96-
public void PositionShouldReturnNumberOfBytesWrittenToBuffer()
97+
public void PositionShouldReturnNumberOfBytesWrittenToCallerProvidedBuffer()
9798
{
9899
SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
99100

100-
Assert.AreEqual(_buffer.Length, _target.Position);
101+
Assert.AreEqual(_actual, _target.Position);
101102

102103
SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
103104
}
104105

105106
[TestMethod]
106-
public void ReadShouldReturnAllRemaningBytesFromReadBufferWhenCountIsEqualToNumberOfRemainingBytes()
107+
public void SubsequentReadShouldReturnAllRemaningBytesFromReadBufferWhenCountIsEqualToNumberOfRemainingBytes()
107108
{
108109
SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
109110

110-
_buffer = new byte[_numberOfBytesToWriteToReadBuffer];
111+
var buffer = new byte[_numberOfBytesToWriteToReadBuffer];
111112

112-
var actual = _target.Read(_buffer, 0, _numberOfBytesToWriteToReadBuffer);
113+
var actual = _target.Read(buffer, 0, _numberOfBytesToWriteToReadBuffer);
113114

114115
Assert.AreEqual(_numberOfBytesToWriteToReadBuffer, actual);
115-
Assert.IsTrue(_serverData.Take(_numberOfBytesToRead, _numberOfBytesToWriteToReadBuffer).IsEqualTo(_buffer));
116+
Assert.IsTrue(_serverData.Take(_numberOfBytesToRead, _numberOfBytesToWriteToReadBuffer).IsEqualTo(buffer));
116117

117118
SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
118119
}
120+
121+
[TestMethod]
122+
public void SubsequentReadShouldReturnAllRemaningBytesFromReadBufferAndReadAgainWhenCountIsGreaterThanNumberOfRemainingBytesAndNewReadReturnsZeroBytes()
123+
{
124+
SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
125+
SftpSessionMock.InSequence(MockSequence).Setup(p => p.RequestRead(_handle, (ulong)(_serverData.Length), _readBufferSize)).Returns(Array<byte>.Empty);
126+
127+
var buffer = new byte[_numberOfBytesToWriteToReadBuffer + 1];
128+
129+
var actual = _target.Read(buffer, 0, buffer.Length);
130+
131+
Assert.AreEqual(_numberOfBytesToWriteToReadBuffer, actual);
132+
Assert.IsTrue(_serverData.Take(_numberOfBytesToRead, _numberOfBytesToWriteToReadBuffer).IsEqualTo(buffer.Take(_numberOfBytesToWriteToReadBuffer)));
133+
Assert.AreEqual(0, buffer[_numberOfBytesToWriteToReadBuffer]);
134+
135+
SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
136+
SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong)(_serverData.Length), _readBufferSize));
137+
}
119138
}
120139
}

src/Renci.SshNet.Tests/Common/Extensions.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using System.Collections.Generic;
22
using Renci.SshNet.Common;
3+
using System;
34

45
namespace Renci.SshNet.Tests.Common
56
{
@@ -16,5 +17,12 @@ public static string AsString(this IList<ExceptionEventArgs> exceptionEvents)
1617

1718
return reportedExceptions;
1819
}
20+
21+
public static byte[] Copy(this byte[] buffer)
22+
{
23+
var copy = new byte[buffer.Length];
24+
Buffer.BlockCopy(buffer, 0, copy, 0, buffer.Length);
25+
return copy;
26+
}
1927
}
2028
}

src/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,8 @@
434434
<Compile Include="Classes\Sftp\SftpFileStreamTest_Dispose_Disposed.cs" />
435435
<Compile Include="Classes\Sftp\SftpFileStreamTest_Finalize_SessionOpen.cs" />
436436
<Compile Include="Classes\Sftp\SftpFileStreamTest_ReadByte_ReadMode_NoDataInWriteBufferAndNoDataInReadBuffer_LessDataThanReadBufferSizeAvailable.cs" />
437-
<Compile Include="Classes\Sftp\SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCount.cs" />
437+
<Compile Include="Classes\Sftp\SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCountAndLessThanBufferSize.cs" />
438+
<Compile Include="Classes\Sftp\SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCountAndEqualToBufferSize.cs" />
438439
<Compile Include="Classes\Sftp\SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadMoreBytesFromServerThanCount.cs" />
439440
<Compile Include="Classes\Sftp\SftpFileStreamTest_Seek_PositionedAtBeginningOfStream_OriginBeginAndOffsetNegative.cs" />
440441
<Compile Include="Classes\Sftp\SftpFileStreamTest_Seek_PositionedAtBeginningOfStream_OriginBeginAndOffsetPositive.cs" />

0 commit comments

Comments
 (0)