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.

src/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCount.cs → src/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCountAndEqualToBufferSize.cs

@@ -8,7 +8,7 @@ using Renci.SshNet.Sftp;
 namespace Renci.SshNet.Tests.Classes.Sftp
 {
     [TestClass]
-    public class SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCount : SftpFileStreamTestBase
+    public class SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCountAndEqualToBufferSize : SftpFileStreamTestBase
     {
         private string _path;
         private SftpFileStream _target;
@@ -35,12 +35,14 @@ namespace Renci.SshNet.Tests.Classes.Sftp
             _readBufferSize = 20;
             _writeBufferSize = 500;
 
-            _numberOfBytesToRead = 20;
+            _numberOfBytesToRead = (int) _readBufferSize + 5; // greather than read buffer size
             _buffer = new byte[_numberOfBytesToRead];
-            _serverData1Length = _numberOfBytesToRead - 5;
+            _serverData1Length = (int) _readBufferSize; // equal to read buffer size
             _serverData1 = GenerateRandom(_serverData1Length, random);
-            _serverData2Length = _numberOfBytesToRead - _serverData1Length + 3;
+            _serverData2Length = (int) _readBufferSize; // equal to read buffer size
             _serverData2 = GenerateRandom(_serverData2Length, random);
+
+            Assert.IsTrue(_serverData1Length < _numberOfBytesToRead && _serverData1Length == _readBufferSize);
         }
 
         protected override void SetupMocks()
@@ -61,7 +63,7 @@ namespace Renci.SshNet.Tests.Classes.Sftp
                 .Setup(p => p.RequestRead(_handle, 0UL, _readBufferSize))
                 .Returns(_serverData1);
             SftpSessionMock.InSequence(MockSequence)
-                .Setup(p => p.RequestRead(_handle, (ulong) _serverData1.Length, _readBufferSize))
+                .Setup(p => p.RequestRead(_handle, (ulong)_serverData1.Length, _readBufferSize))
                 .Returns(_serverData2);
         }
 
@@ -91,7 +93,7 @@ namespace Renci.SshNet.Tests.Classes.Sftp
         [TestMethod]
         public void ReadShouldHaveReturnedTheNumberOfBytesRequested()
         {
-            Assert.AreEqual(_buffer.Length, _actual);
+            Assert.AreEqual(_numberOfBytesToRead, _actual);
         }
 
         [TestMethod]
@@ -104,11 +106,11 @@ namespace Renci.SshNet.Tests.Classes.Sftp
         }
 
         [TestMethod]
-        public void PositionShouldReturnNumberOfBytesWrittenToBuffer()
+        public void PositionShouldReturnNumberOfBytesWrittenToCallerProvidedBuffer()
         {
             SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
 
-            Assert.AreEqual(_buffer.Length, _target.Position);
+            Assert.AreEqual(_actual, _target.Position);
 
             SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
         }
@@ -134,7 +136,7 @@ namespace Renci.SshNet.Tests.Classes.Sftp
         public void ReadShouldReturnAllRemaningBytesFromReadBufferAndReadAgainWhenCountIsGreaterThanNumberOfRemainingBytesAndNewReadReturnsZeroBytes()
         {
             SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
-            SftpSessionMock.InSequence(MockSequence).Setup(p => p.RequestRead(_handle, (ulong) (_serverData1Length + _serverData2Length), _readBufferSize)).Returns(Array<byte>.Empty);
+            SftpSessionMock.InSequence(MockSequence).Setup(p => p.RequestRead(_handle, (ulong)(_serverData1Length + _serverData2Length), _readBufferSize)).Returns(Array<byte>.Empty);
 
             var numberOfBytesRemainingInReadBuffer = _serverData1Length + _serverData2Length - _numberOfBytesToRead;
 
@@ -147,7 +149,7 @@ namespace Renci.SshNet.Tests.Classes.Sftp
             Assert.AreEqual(0, _buffer[numberOfBytesRemainingInReadBuffer]);
 
             SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
-            SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong) (_serverData1Length + _serverData2Length), _readBufferSize));
+            SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong)(_serverData1Length + _serverData2Length), _readBufferSize));
         }
     }
 }

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

@@ -0,0 +1,148 @@
+using System;
+using System.IO;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Moq;
+using Renci.SshNet.Common;
+using Renci.SshNet.Sftp;
+using Renci.SshNet.Tests.Common;
+
+namespace Renci.SshNet.Tests.Classes.Sftp
+{
+    [TestClass]
+    public class SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCountAndLessThanBufferSize : SftpFileStreamTestBase
+    {
+        private string _path;
+        private SftpFileStream _target;
+        private byte[] _handle;
+        private uint _bufferSize;
+        private uint _readBufferSize;
+        private uint _writeBufferSize;
+        private int _actual;
+        private byte[] _buffer;
+        private byte[] _serverData;
+        private int _serverDataLength;
+        private int _numberOfBytesToRead;
+        private byte[] _originalBuffer;
+
+        protected override void SetupData()
+        {
+            base.SetupData();
+
+            var random = new Random();
+            _path = random.Next().ToString();
+            _handle = GenerateRandom(5, random);
+            _bufferSize = (uint)random.Next(1, 1000);
+            _readBufferSize = 20;
+            _writeBufferSize = 500;
+
+            _numberOfBytesToRead = (int) _readBufferSize + 2; // greater than read buffer size
+            _originalBuffer = GenerateRandom(_numberOfBytesToRead, random);
+            _buffer = _originalBuffer.Copy();
+
+            _serverDataLength = (int) _readBufferSize - 1; // less than read buffer size
+            _serverData = GenerateRandom(_serverDataLength, random);
+
+            Assert.IsTrue(_serverDataLength < _numberOfBytesToRead && _serverDataLength < _readBufferSize);
+        }
+
+        protected override void SetupMocks()
+        {
+            SftpSessionMock.InSequence(MockSequence)
+                .Setup(p => p.RequestOpen(_path, Flags.Read, false))
+                .Returns(_handle);
+            SftpSessionMock.InSequence(MockSequence)
+                .Setup(p => p.CalculateOptimalReadLength(_bufferSize))
+                .Returns(_readBufferSize);
+            SftpSessionMock.InSequence(MockSequence)
+                .Setup(p => p.CalculateOptimalWriteLength(_bufferSize, _handle))
+                .Returns(_writeBufferSize);
+            SftpSessionMock.InSequence(MockSequence)
+                .Setup(p => p.IsOpen)
+                .Returns(true);
+            SftpSessionMock.InSequence(MockSequence)
+                .Setup(p => p.RequestRead(_handle, 0UL, _readBufferSize))
+                .Returns(_serverData);
+        }
+
+        [TestCleanup]
+        public void TearDown()
+        {
+            SftpSessionMock.InSequence(MockSequence)
+                           .Setup(p => p.RequestClose(_handle));
+        }
+
+        protected override void Arrange()
+        {
+            base.Arrange();
+
+            _target = new SftpFileStream(SftpSessionMock.Object,
+                                         _path,
+                                         FileMode.Open,
+                                         FileAccess.Read,
+                                         (int)_bufferSize);
+        }
+
+        protected override void Act()
+        {
+            _actual = _target.Read(_buffer, 0, _numberOfBytesToRead);
+        }
+
+        [TestMethod]
+        public void ReadShouldHaveReturnedTheNumberOfBytesReturnedByTheReadFromTheServer()
+        {
+            Assert.AreEqual(_serverDataLength, _actual);
+        }
+
+        [TestMethod]
+        public void ReadShouldHaveWrittenBytesToTheCallerSuppliedBufferAndRemainingBytesShouldRemainUntouched()
+        {
+            Assert.IsTrue(_serverData.IsEqualTo(_buffer.Take(_serverDataLength)));
+            Assert.IsTrue(_originalBuffer.Take(_serverDataLength, _originalBuffer.Length - _serverDataLength).IsEqualTo(_buffer.Take(_serverDataLength, _buffer.Length - _serverDataLength)));
+        }
+
+        [TestMethod]
+        public void PositionShouldReturnNumberOfBytesWrittenToCallerProvidedBuffer()
+        {
+            SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
+
+            Assert.AreEqual(_actual, _target.Position);
+
+            SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
+        }
+
+        [TestMethod]
+        public void SubsequentReadShouldReadAgainFromCurrentPositionFromServerAndReturnZeroWhenServerReturnsZeroBytes()
+        {
+            SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
+            SftpSessionMock.InSequence(MockSequence)
+                .Setup(p => p.RequestRead(_handle, (ulong) _actual, _readBufferSize))
+                .Returns(Array<byte>.Empty);
+
+            var buffer = _originalBuffer.Copy();
+            var actual = _target.Read(buffer, 0, buffer.Length);
+
+            Assert.AreEqual(0, actual);
+            Assert.IsTrue(_originalBuffer.IsEqualTo(buffer));
+
+            SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong)_actual, _readBufferSize), Times.Once);
+            SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
+        }
+
+        [TestMethod]
+        public void SubsequentReadShouldReadAgainFromCurrentPositionFromServerAndNotUpdatePositionWhenServerReturnsZeroBytes()
+        {
+            SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
+            SftpSessionMock.InSequence(MockSequence)
+                .Setup(p => p.RequestRead(_handle, (ulong)_actual, _readBufferSize))
+                .Returns(Array<byte>.Empty);
+            SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
+
+            _target.Read(new byte[10], 0, 10);
+
+            Assert.AreEqual(_actual, _target.Position);
+
+            SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong)_actual, _readBufferSize), Times.Once);
+            SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(3));
+        }
+    }
+}

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

@@ -3,6 +3,7 @@ using System.IO;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using Moq;
 using Renci.SshNet.Sftp;
+using Renci.SshNet.Common;
 
 namespace Renci.SshNet.Tests.Classes.Sftp
 {
@@ -81,7 +82,7 @@ namespace Renci.SshNet.Tests.Classes.Sftp
         }
 
         [TestMethod]
-        public void ReadShouldHaveReturnedTheNumberOfBytesRequested()
+        public void ReadShouldHaveReturnedTheNumberOfBytesWrittenToCallerSuppliedBuffer()
         {
             Assert.AreEqual(_numberOfBytesToRead, _actual);
         }
@@ -89,32 +90,50 @@ namespace Renci.SshNet.Tests.Classes.Sftp
         [TestMethod]
         public void ReadShouldHaveWrittenBytesToTheCallerSuppliedBuffer()
         {
-            Assert.IsTrue(_serverData.Take(_buffer.Length).IsEqualTo(_buffer));
+            Assert.IsTrue(_serverData.Take(_actual).IsEqualTo(_buffer));
         }
 
         [TestMethod]
-        public void PositionShouldReturnNumberOfBytesWrittenToBuffer()
+        public void PositionShouldReturnNumberOfBytesWrittenToCallerProvidedBuffer()
         {
             SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
 
-            Assert.AreEqual(_buffer.Length, _target.Position);
+            Assert.AreEqual(_actual, _target.Position);
 
             SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
         }
 
         [TestMethod]
-        public void ReadShouldReturnAllRemaningBytesFromReadBufferWhenCountIsEqualToNumberOfRemainingBytes()
+        public void SubsequentReadShouldReturnAllRemaningBytesFromReadBufferWhenCountIsEqualToNumberOfRemainingBytes()
         {
             SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
 
-            _buffer = new byte[_numberOfBytesToWriteToReadBuffer];
+            var buffer = new byte[_numberOfBytesToWriteToReadBuffer];
 
-            var actual = _target.Read(_buffer, 0, _numberOfBytesToWriteToReadBuffer);
+            var actual = _target.Read(buffer, 0, _numberOfBytesToWriteToReadBuffer);
 
             Assert.AreEqual(_numberOfBytesToWriteToReadBuffer, actual);
-            Assert.IsTrue(_serverData.Take(_numberOfBytesToRead, _numberOfBytesToWriteToReadBuffer).IsEqualTo(_buffer));
+            Assert.IsTrue(_serverData.Take(_numberOfBytesToRead, _numberOfBytesToWriteToReadBuffer).IsEqualTo(buffer));
 
             SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
         }
+
+        [TestMethod]
+        public void SubsequentReadShouldReturnAllRemaningBytesFromReadBufferAndReadAgainWhenCountIsGreaterThanNumberOfRemainingBytesAndNewReadReturnsZeroBytes()
+        {
+            SftpSessionMock.InSequence(MockSequence).Setup(p => p.IsOpen).Returns(true);
+            SftpSessionMock.InSequence(MockSequence).Setup(p => p.RequestRead(_handle, (ulong)(_serverData.Length), _readBufferSize)).Returns(Array<byte>.Empty);
+
+            var buffer = new byte[_numberOfBytesToWriteToReadBuffer + 1];
+
+            var actual = _target.Read(buffer, 0, buffer.Length);
+
+            Assert.AreEqual(_numberOfBytesToWriteToReadBuffer, actual);
+            Assert.IsTrue(_serverData.Take(_numberOfBytesToRead, _numberOfBytesToWriteToReadBuffer).IsEqualTo(buffer.Take(_numberOfBytesToWriteToReadBuffer)));
+            Assert.AreEqual(0, buffer[_numberOfBytesToWriteToReadBuffer]);
+
+            SftpSessionMock.Verify(p => p.IsOpen, Times.Exactly(2));
+            SftpSessionMock.Verify(p => p.RequestRead(_handle, (ulong)(_serverData.Length), _readBufferSize));
+        }
     }
 }

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

@@ -1,5 +1,6 @@
 using System.Collections.Generic;
 using Renci.SshNet.Common;
+using System;
 
 namespace Renci.SshNet.Tests.Common
 {
@@ -16,5 +17,12 @@ namespace Renci.SshNet.Tests.Common
 
             return reportedExceptions;
         }
+
+        public static byte[] Copy(this byte[] buffer)
+        {
+            var copy = new byte[buffer.Length];
+            Buffer.BlockCopy(buffer, 0, copy, 0, buffer.Length);
+            return copy;
+        }
     }
 }

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

@@ -434,7 +434,8 @@
     <Compile Include="Classes\Sftp\SftpFileStreamTest_Dispose_Disposed.cs" />
     <Compile Include="Classes\Sftp\SftpFileStreamTest_Finalize_SessionOpen.cs" />
     <Compile Include="Classes\Sftp\SftpFileStreamTest_ReadByte_ReadMode_NoDataInWriteBufferAndNoDataInReadBuffer_LessDataThanReadBufferSizeAvailable.cs" />
-    <Compile Include="Classes\Sftp\SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCount.cs" />
+    <Compile Include="Classes\Sftp\SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCountAndLessThanBufferSize.cs" />
+    <Compile Include="Classes\Sftp\SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadLessBytesFromServerThanCountAndEqualToBufferSize.cs" />
     <Compile Include="Classes\Sftp\SftpFileStreamTest_Read_ReadMode_NoDataInReaderBufferAndReadMoreBytesFromServerThanCount.cs" />
     <Compile Include="Classes\Sftp\SftpFileStreamTest_Seek_PositionedAtBeginningOfStream_OriginBeginAndOffsetNegative.cs" />
     <Compile Include="Classes\Sftp\SftpFileStreamTest_Seek_PositionedAtBeginningOfStream_OriginBeginAndOffsetPositive.cs" />

src/Renci.SshNet/Sftp/SftpFileStream.cs

@@ -310,6 +310,22 @@ namespace Renci.SshNet.Sftp
         /// <exception cref="IOException">An I/O error occurs. </exception>
         /// <exception cref="NotSupportedException">The stream does not support reading. </exception>
         /// <exception cref="ObjectDisposedException">Methods were called after the stream was closed. </exception>
+        /// <remarks>
+        /// <para>
+        /// This method attempts to read up to <paramref name="count"/> bytes. This either from the buffer, from the
+        /// server (using one or more <c>SSH_FXP_READ</c> requests) or using a combination of both.
+        /// </para>
+        /// <para>
+        /// The read loop is interrupted when either <paramref name="count"/> bytes are read, the server returns zero
+        /// bytes (EOF) or less bytes than the read buffer size.
+        /// </para>
+        /// <para>
+        /// When a server returns less number of bytes than the read buffer size, this <c>may</c> indicate that EOF has
+        /// been reached. A subsequent (<c>SSH_FXP_READ</c>) 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 <see cref="Read"/> or <see cref="ReadByte"/> invocation.
+        /// </para>
+        /// </remarks>
         public override int Read(byte[] buffer, int offset, int count)
         {
             var readLen = 0;
@@ -371,14 +387,25 @@ namespace Renci.SshNet.Sftp
 
                         // write bytes to caller-provided buffer
                         Buffer.BlockCopy(data, 0, buffer, offset, bytesToWriteToCallerBuffer);
-                        // advance offset to start writing bytes into caller-provided buffer
-                        offset += bytesToWriteToCallerBuffer;
+                        // update stream position
+                        _position += bytesToWriteToCallerBuffer;
                         // record total number of bytes read into caller-provided buffer
                         readLen += bytesToWriteToCallerBuffer;
-                        // signal that all caller-requested bytes are read
+
+                        // break out of the read loop when the server returned less than the request number of bytes
+                        // as that *may* indicate that we've reached EOF
+                        //
+                        // doing this avoids reading from server twice to determine EOF: once in the read loop, and
+                        // once upon the next Read or ReadByte invocation by the caller
+                        if (data.Length < _readBufferSize)
+                        {
+                            break;
+                        }
+
+                        // advance offset to start writing bytes into caller-provided buffer
+                        offset += bytesToWriteToCallerBuffer;
+                        // update number of bytes left to read into caller-provided buffer
                         count -= bytesToWriteToCallerBuffer;
-                        // update stream position
-                        _position += bytesToWriteToCallerBuffer;
                     }
                     else
                     {
@@ -390,14 +417,14 @@ namespace Renci.SshNet.Sftp
                         Buffer.BlockCopy(GetOrCreateReadBuffer(), _bufferPosition, buffer, offset, bytesAvailableInBuffer);
                         // update position in read buffer
                         _bufferPosition += bytesAvailableInBuffer;
-                        // advance offset to start writing bytes into caller-provided buffer
-                        offset += bytesAvailableInBuffer;
+                        // update stream position
+                        _position += bytesAvailableInBuffer;
                         // record total number of bytes read into caller-provided buffer
                         readLen += bytesAvailableInBuffer;
+                        // advance offset to start writing bytes into caller-provided buffer
+                        offset += bytesAvailableInBuffer;
                         // update number of bytes left to read
                         count -= bytesAvailableInBuffer;
-                        // update stream position
-                        _position += bytesAvailableInBuffer;
                     }
                 }
             }