From f3bb926a35606f3484049b4c8291e354d3faa9c0 Mon Sep 17 00:00:00 2001 From: Robert Hague Date: Fri, 1 Aug 2025 18:14:53 +0200 Subject: [PATCH 1/2] Use ReadExactly in ReadAllBytes and yield in ReadLines --- src/Renci.SshNet/Common/Extensions.cs | 21 + src/Renci.SshNet/SftpClient.cs | 59 +-- .../SftpTests.cs | 389 ++++-------------- 3 files changed, 142 insertions(+), 327 deletions(-) diff --git a/src/Renci.SshNet/Common/Extensions.cs b/src/Renci.SshNet/Common/Extensions.cs index 14b80ac74..83ebb860c 100644 --- a/src/Renci.SshNet/Common/Extensions.cs +++ b/src/Renci.SshNet/Common/Extensions.cs @@ -2,6 +2,9 @@ using System.Collections.Generic; using System.Diagnostics; using System.Globalization; +#if !NET +using System.IO; +#endif using System.Net; using System.Net.Sockets; using System.Numerics; @@ -406,6 +409,24 @@ internal static T[] ToArray(this ArraySegment arraySegment) Array.Copy(arraySegment.Array, arraySegment.Offset, array, 0, arraySegment.Count); return array; } + +#pragma warning disable CA1859 // Use concrete types for improved performance + internal static void ReadExactly(this Stream stream, byte[] buffer, int offset, int count) +#pragma warning restore CA1859 + { + var totalRead = 0; + + while (totalRead < count) + { + var read = stream.Read(buffer, offset + totalRead, count - totalRead); + if (read == 0) + { + throw new EndOfStreamException(); + } + + totalRead += read; + } + } #endif } } diff --git a/src/Renci.SshNet/SftpClient.cs b/src/Renci.SshNet/SftpClient.cs index 1869bcaa7..714652920 100644 --- a/src/Renci.SshNet/SftpClient.cs +++ b/src/Renci.SshNet/SftpClient.cs @@ -5,6 +5,7 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.IO; +using System.Linq; using System.Net; using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; @@ -1712,7 +1713,7 @@ public byte[] ReadAllBytes(string path) using (var stream = OpenRead(path)) { var buffer = new byte[stream.Length]; - _ = stream.Read(buffer, 0, buffer.Length); + stream.ReadExactly(buffer, 0, buffer.Length); return buffer; } } @@ -1745,23 +1746,7 @@ public string[] ReadAllLines(string path) /// The method was called after the client was disposed. public string[] ReadAllLines(string path, Encoding encoding) { - /* - * We use the default buffer size for StreamReader - which is 1024 bytes - and the configured buffer size - * for the SftpFileStream. We may want to revisit this later. - */ - - var lines = new List(); - - using (var stream = new StreamReader(OpenRead(path), encoding)) - { - string? line; - while ((line = stream.ReadLine()) != null) - { - lines.Add(line); - } - } - - return lines.ToArray(); + return ReadLines(path, encoding).ToArray(); } /// @@ -1792,15 +1777,8 @@ public string ReadAllText(string path) /// The method was called after the client was disposed. public string ReadAllText(string path, Encoding encoding) { - /* - * We use the default buffer size for StreamReader - which is 1024 bytes - and the configured buffer size - * for the SftpFileStream. We may want to revisit this later. - */ - - using (var stream = new StreamReader(OpenRead(path), encoding)) - { - return stream.ReadToEnd(); - } + using var sr = new StreamReader(OpenRead(path), encoding); + return sr.ReadToEnd(); } /// @@ -1815,7 +1793,7 @@ public string ReadAllText(string path, Encoding encoding) /// The method was called after the client was disposed. public IEnumerable ReadLines(string path) { - return ReadAllLines(path); + return ReadLines(path, Encoding.UTF8); } /// @@ -1831,7 +1809,30 @@ public IEnumerable ReadLines(string path) /// The method was called after the client was disposed. public IEnumerable ReadLines(string path, Encoding encoding) { - return ReadAllLines(path, encoding); + // We open the file eagerly i.e. outside of the state machine created by yield, + // in order to a) throw resulting (e.g. file-related) exceptions eagerly; and b) + // to match what File.ReadLines does. + // This probably makes it behave more predictably/closer to what most people expect. + // The downside is that if the return value is never enumerated, the file + // is never closed (we can't do "using" here because it would be disposed + // as soon as we return). This conundrum also exists with File.ReadLines. + + var sr = new StreamReader(OpenRead(path), encoding); + + return Enumerate(sr); + + static IEnumerable Enumerate(StreamReader sr) + { + using (sr) + { + string? line; + + while ((line = sr.ReadLine()) != null) + { + yield return line; + } + } + } } /// diff --git a/test/Renci.SshNet.IntegrationTests/SftpTests.cs b/test/Renci.SshNet.IntegrationTests/SftpTests.cs index 64ca1521b..c4d48886f 100644 --- a/test/Renci.SshNet.IntegrationTests/SftpTests.cs +++ b/test/Renci.SshNet.IntegrationTests/SftpTests.cs @@ -214,7 +214,7 @@ public void Sftp_Create_ExistingFile() } var actualContent1 = client.ReadAllBytes(remoteFile); - Assert.IsTrue(newContent1Bytes.IsEqualTo(actualContent1)); + CollectionAssert.AreEqual(newContent1Bytes, actualContent1); #endregion Write less bytes than the current content, overwriting part of that content @@ -227,7 +227,7 @@ public void Sftp_Create_ExistingFile() } var actualContent2 = client.ReadAllBytes(remoteFile); - Assert.IsTrue(newContent2Bytes.IsEqualTo(actualContent2)); + CollectionAssert.AreEqual(newContent2Bytes, actualContent2); #endregion Write more bytes than the current content, overwriting and appending to that content } @@ -726,12 +726,8 @@ public void Sftp_AppendAllLines_Encoding_ExistingFile() var text = client.ReadAllText(remoteFile, encoding); Assert.AreEqual(expectedContent, text); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes, actualBytes); } finally { @@ -807,12 +803,8 @@ public void Sftp_AppendAllLines_Encoding_FileDoesNotExist() var text = client.ReadAllText(remoteFile, encoding); Assert.AreEqual(expectedContent, text); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes, actualBytes); } finally { @@ -852,13 +844,8 @@ public void Sftp_AppendAllText_Encoding_ExistingFile() var text = client.ReadAllText(remoteFile, encoding); Assert.AreEqual(expectedContent, text); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - - Assert.IsTrue(expectedBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes, actualBytes); } finally { @@ -933,12 +920,8 @@ public void Sftp_AppendAllText_Encoding_FileDoesNotExist() var text = client.ReadAllText(remoteFile, encoding); Assert.AreEqual(expectedContent, text); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes, actualBytes); } finally { @@ -982,12 +965,8 @@ public void Sftp_AppendText_Encoding_ExistingFile() var text = client.ReadAllText(remoteFile, encoding); Assert.AreEqual(expectedContent, text); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes, actualBytes); } finally { @@ -1064,12 +1043,8 @@ public void Sftp_AppendText_Encoding_FileDoesNotExist() sw.Write(contentToAppend); } - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes, actualBytes); } finally { @@ -1111,12 +1086,8 @@ public void Sftp_CreateText_NoEncoding_ExistingFile() } // verify that original content is left untouched - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(initialContentBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(initialContentBytes, actualBytes); // write content that is less bytes than original content using (var sw = client.CreateText(remoteFile)) @@ -1128,12 +1099,8 @@ public void Sftp_CreateText_NoEncoding_ExistingFile() var text = client.ReadAllText(remoteFile); Assert.AreEqual(expectedContent, text); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedContentBytes.IsEqualTo(actualBytes)); - } + actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedContentBytes, actualBytes); } finally { @@ -1224,12 +1191,8 @@ public void Sftp_CreateText_NoEncoding_FileDoesNotExist() var text = client.ReadAllText(remoteFile); Assert.AreEqual(initialContent, text); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(initialContentBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(initialContentBytes, actualBytes); } finally { @@ -1271,12 +1234,8 @@ public void Sftp_CreateText_Encoding_ExistingFile() } // verify that original content is left untouched - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(initialContentBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(initialContentBytes, actualBytes); // write content that is less bytes than original content using (var sw = client.CreateText(remoteFile, encoding)) @@ -1288,12 +1247,8 @@ public void Sftp_CreateText_Encoding_ExistingFile() var text = client.ReadAllText(remoteFile, encoding); Assert.AreEqual(expectedContent, text); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedContentBytes.IsEqualTo(actualBytes)); - } + actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedContentBytes, actualBytes); } finally { @@ -1386,12 +1341,8 @@ public void Sftp_CreateText_Encoding_FileDoesNotExist() var text = client.ReadAllText(remoteFile, encoding); Assert.AreEqual(initialContent, text); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(initialContentBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(initialContentBytes, actualBytes); } finally { @@ -1443,41 +1394,6 @@ public void Sftp_DownloadFile_FileDoesNotExist() } } - [TestMethod] - public void Sftp_ReadAllBytes_ExistingFile() - { - var encoding = GetRandomEncoding(); - var content = "\u0100ert & Ann"; - var contentBytes = GetBytesWithPreamble(content, encoding); - - using (var client = new SftpClient(_connectionInfoFactory.Create())) - { - client.Connect(); - - var remoteFile = GenerateUniqueRemoteFileName(); - - if (client.Exists(remoteFile)) - { - client.DeleteFile(remoteFile); - } - - try - { - client.WriteAllText(remoteFile, content, encoding); - - var actualBytes = client.ReadAllBytes(remoteFile); - Assert.IsTrue(contentBytes.IsEqualTo(actualBytes)); - } - finally - { - if (client.Exists(remoteFile)) - { - client.DeleteFile(remoteFile); - } - } - } - } - [TestMethod] public void Sftp_ReadAllBytes_FileDoesNotExist() { @@ -1546,19 +1462,10 @@ public void Sftp_ReadAllLines_NoEncoding_ExistingFile() var actualLines = client.ReadAllLines(remoteFile); Assert.IsNotNull(actualLines); - Assert.AreEqual(lines.Length, actualLines.Length); - - for (var i = 0; i < lines.Length; i++) - { - Assert.AreEqual(lines[i], actualLines[i]); - } + CollectionAssert.AreEqual(lines, actualLines); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(linesBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(linesBytes, actualBytes); } finally { @@ -1638,19 +1545,10 @@ public void Sftp_ReadAllLines_Encoding_ExistingFile() var actualLines = client.ReadAllLines(remoteFile, encoding); Assert.IsNotNull(actualLines); - Assert.AreEqual(lines.Length, actualLines.Length); + CollectionAssert.AreEqual(lines, actualLines); - for (var i = 0; i < lines.Length; i++) - { - Assert.AreEqual(lines[i], actualLines[i]); - } - - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(linesBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(linesBytes, actualBytes); } finally { @@ -1733,12 +1631,8 @@ public void Sftp_ReadAllText_NoEncoding_ExistingFile() var actualText = client.ReadAllText(remoteFile); Assert.AreEqual(expectedText, actualText); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes, actualBytes); } finally { @@ -1819,12 +1713,8 @@ public void Sftp_ReadAllText_Encoding_ExistingFile() var actualText = client.ReadAllText(remoteFile, encoding); Assert.AreEqual(expectedText, actualText); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes, actualBytes); } finally { @@ -1903,16 +1793,7 @@ public void Sftp_ReadLines_NoEncoding_ExistingFile() var actualLines = client.ReadLines(remoteFile); Assert.IsNotNull(actualLines); - - var actualLinesEnum = actualLines.GetEnumerator(); - for (var i = 0; i < lines.Length; i++) - { - Assert.IsTrue(actualLinesEnum.MoveNext()); - var actualLine = actualLinesEnum.Current; - Assert.AreEqual(lines[i], actualLine); - } - - Assert.IsFalse(actualLinesEnum.MoveNext()); + CollectionAssert.AreEqual(lines, actualLines.ToArray()); } finally { @@ -1990,19 +1871,7 @@ public void Sftp_ReadLines_Encoding_ExistingFile() var actualLines = client.ReadLines(remoteFile, encoding); Assert.IsNotNull(actualLines); - - using (var actualLinesEnum = actualLines.GetEnumerator()) - { - for (var i = 0; i < lines.Length; i++) - { - Assert.IsTrue(actualLinesEnum.MoveNext()); - - var actualLine = actualLinesEnum.Current; - Assert.AreEqual(lines[i], actualLine); - } - - Assert.IsFalse(actualLinesEnum.MoveNext()); - } + CollectionAssert.AreEqual(lines, actualLines.ToArray()); } finally { @@ -2122,7 +1991,7 @@ public void Sftp_WriteAllBytes_ExistingFile() client.WriteAllBytes(remoteFile, newContent1); var actualContent1 = client.ReadAllBytes(remoteFile); - Assert.IsTrue(expectedContent1.IsEqualTo(actualContent1)); + CollectionAssert.AreEqual(expectedContent1, actualContent1); #endregion Write less bytes than the initial content, overwriting part of that content @@ -2131,7 +2000,7 @@ public void Sftp_WriteAllBytes_ExistingFile() client.WriteAllBytes(remoteFile, newContent2); var actualContent2 = client.ReadAllBytes(remoteFile); - Assert.IsTrue(newContent2.IsEqualTo(actualContent2)); + CollectionAssert.AreEqual(newContent2, actualContent2); #endregion Write less bytes than the initial content, overwriting part of that content } @@ -2165,12 +2034,8 @@ public void Sftp_WriteAllBytes_FileDoesNotExist() { client.WriteAllBytes(remoteFile, content); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(content.IsEqualTo(actualBytes)); - } + var actualContent1 = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(content, actualContent1); } finally { @@ -2258,12 +2123,8 @@ public void Sftp_WriteAllLines_IEnumerable_NoEncoding_ExistingFile() client.WriteAllLines(remoteFile, linesToWrite1); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes1.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes1, actualBytes); #endregion Write less bytes than the current content, overwriting part of that content @@ -2271,12 +2132,8 @@ public void Sftp_WriteAllLines_IEnumerable_NoEncoding_ExistingFile() client.WriteAllLines(remoteFile, linesToWrite2); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes2.IsEqualTo(actualBytes)); - } + actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes2, actualBytes); #endregion Write more bytes than the current content, overwriting and appending to that content } @@ -2312,12 +2169,8 @@ public void Sftp_WriteAllLines_IEnumerable_NoEncoding_FileDoesNotExist() { client.WriteAllLines(remoteFile, linesToWrite); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(linesToWriteBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(linesToWriteBytes, actualBytes); } finally { @@ -2404,12 +2257,8 @@ public void Sftp_WriteAllLines_IEnumerable_Encoding_ExistingFile() client.WriteAllLines(remoteFile, linesToWrite1, encoding); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes1.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes1, actualBytes); #endregion Write less bytes than the current content, overwriting part of that content @@ -2417,12 +2266,8 @@ public void Sftp_WriteAllLines_IEnumerable_Encoding_ExistingFile() client.WriteAllLines(remoteFile, linesToWrite2, encoding); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes2.IsEqualTo(actualBytes)); - } + actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes2, actualBytes); #endregion Write more bytes than the current content, overwriting and appending to that content } @@ -2458,12 +2303,8 @@ public void Sftp_WriteAllLines_IEnumerable_Encoding_FileDoesNotExist() { client.WriteAllLines(remoteFile, linesToWrite, encoding); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(linesToWriteBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(linesToWriteBytes, actualBytes); } finally { @@ -2546,12 +2387,8 @@ public void Sftp_WriteAllLines_Array_NoEncoding_ExistingFile() client.WriteAllLines(remoteFile, linesToWrite1); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes1.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes1, actualBytes); #endregion Write less bytes than the current content, overwriting part of that content @@ -2559,12 +2396,8 @@ public void Sftp_WriteAllLines_Array_NoEncoding_ExistingFile() client.WriteAllLines(remoteFile, linesToWrite2); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes2.IsEqualTo(actualBytes)); - } + actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes2, actualBytes); #endregion Write more bytes than the current content, overwriting and appending to that content } @@ -2600,12 +2433,8 @@ public void Sftp_WriteAllLines_Array_NoEncoding_FileDoesNotExist() { client.WriteAllLines(remoteFile, linesToWrite); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(linesToWriteBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(linesToWriteBytes, actualBytes); } finally { @@ -2690,12 +2519,8 @@ public void Sftp_WriteAllLines_Array_Encoding_ExistingFile() client.WriteAllLines(remoteFile, linesToWrite1, encoding); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes1.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes1, actualBytes); #endregion Write less bytes than the current content, overwriting part of that content @@ -2703,12 +2528,8 @@ public void Sftp_WriteAllLines_Array_Encoding_ExistingFile() client.WriteAllLines(remoteFile, linesToWrite2, encoding); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes2.IsEqualTo(actualBytes)); - } + actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes2, actualBytes); #endregion Write more bytes than the current content, overwriting and appending to that content } @@ -2744,12 +2565,8 @@ public void Sftp_WriteAllLines_Array_Encoding_FileDoesNotExist() { client.WriteAllLines(remoteFile, linesToWrite, encoding); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(linesToWriteBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(linesToWriteBytes, actualBytes); } finally { @@ -2833,12 +2650,8 @@ public void Sftp_WriteAllText_NoEncoding_ExistingFile() client.WriteAllText(remoteFile, newContent1); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes1.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes1, actualBytes); #endregion Write less bytes than the current content, overwriting part of that content @@ -2846,12 +2659,8 @@ public void Sftp_WriteAllText_NoEncoding_ExistingFile() client.WriteAllText(remoteFile, newContent2); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes2.IsEqualTo(actualBytes)); - } + actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes2, actualBytes); #endregion Write more bytes than the current content, overwriting and appending to that content } @@ -2888,12 +2697,8 @@ public void Sftp_WriteAllText_NoEncoding_FileDoesNotExist() { client.WriteAllText(remoteFile, initialContent); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(initialContentBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(initialContentBytes, actualBytes); } finally { @@ -2977,12 +2782,8 @@ public void Sftp_WriteAllText_Encoding_ExistingFile() client.WriteAllText(remoteFile, newContent1, encoding); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes1.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes1, actualBytes); #endregion Write less bytes than the current content, overwriting part of that content @@ -2990,12 +2791,8 @@ public void Sftp_WriteAllText_Encoding_ExistingFile() client.WriteAllText(remoteFile, newContent2, encoding); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(expectedBytes2.IsEqualTo(actualBytes)); - } + actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(expectedBytes2, actualBytes); #endregion Write more bytes than the current content, overwriting and appending to that content } @@ -3032,12 +2829,8 @@ public void Sftp_WriteAllText_Encoding_FileDoesNotExist() { client.WriteAllText(remoteFile, initialContent, encoding); - using (var fs = client.OpenRead(remoteFile)) - { - var actualBytes = new byte[fs.Length]; - _ = fs.Read(actualBytes, offset: 0, actualBytes.Length); - Assert.IsTrue(initialContentBytes.IsEqualTo(actualBytes)); - } + var actualBytes = client.ReadAllBytes(remoteFile); + CollectionAssert.AreEqual(initialContentBytes, actualBytes); } finally { @@ -4576,7 +4369,7 @@ public void Sftp_SftpFileStream_Seek_BeyondEndOfFile_SeekOriginBegin() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(readBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -4618,7 +4411,7 @@ public void Sftp_SftpFileStream_Seek_BeyondEndOfFile_SeekOriginBegin() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(readBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -4655,7 +4448,7 @@ public void Sftp_SftpFileStream_Seek_BeyondEndOfFile_SeekOriginBegin() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(writeBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -4695,7 +4488,7 @@ public void Sftp_SftpFileStream_Seek_BeyondEndOfFile_SeekOriginBegin() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(writeBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -4816,7 +4609,7 @@ public void Sftp_SftpFileStream_Seek_BeyondEndOfFile_SeekOriginEnd() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(readBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -4858,7 +4651,7 @@ public void Sftp_SftpFileStream_Seek_BeyondEndOfFile_SeekOriginEnd() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(readBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -4898,7 +4691,7 @@ public void Sftp_SftpFileStream_Seek_BeyondEndOfFile_SeekOriginEnd() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(writeBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -4939,7 +4732,7 @@ public void Sftp_SftpFileStream_Seek_BeyondEndOfFile_SeekOriginEnd() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(writeBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -5026,7 +4819,7 @@ public void Sftp_SftpFileStream_Seek_NegativeOffSet_SeekOriginEnd() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(writeBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -5279,7 +5072,7 @@ public void Sftp_SftpFileStream_Seek_WithinReadBuffer() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(readBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -5321,7 +5114,7 @@ public void Sftp_SftpFileStream_Seek_WithinReadBuffer() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(readBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -5361,7 +5154,7 @@ public void Sftp_SftpFileStream_Seek_WithinReadBuffer() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(writeBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); @@ -5404,7 +5197,7 @@ public void Sftp_SftpFileStream_Seek_WithinReadBuffer() var readBuffer = new byte[writeBuffer.Length]; Assert.AreEqual(writeBuffer.Length, fs.Read(readBuffer, offset: 0, readBuffer.Length)); - Assert.IsTrue(writeBuffer.IsEqualTo(readBuffer)); + CollectionAssert.AreEqual(writeBuffer, readBuffer); // Ensure we've reached end of the stream Assert.AreEqual(-1, fs.ReadByte()); From 501e3400acfb18707e8d50e980c5391b978f441f Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Sat, 2 Aug 2025 15:16:13 +0200 Subject: [PATCH 2/2] on second thought, open the file lazily in ReadLines --- src/Renci.SshNet/SftpClient.cs | 53 +++++++------ .../SftpTests.cs | 74 ++++++++----------- 2 files changed, 58 insertions(+), 69 deletions(-) diff --git a/src/Renci.SshNet/SftpClient.cs b/src/Renci.SshNet/SftpClient.cs index 8b2fe760e..2d3ac574b 100644 --- a/src/Renci.SshNet/SftpClient.cs +++ b/src/Renci.SshNet/SftpClient.cs @@ -1803,9 +1803,13 @@ public string ReadAllText(string path, Encoding encoding) /// /// The lines of the file. /// - /// is . - /// Client is not connected. - /// The method was called after the client was disposed. + /// + /// The lines are enumerated lazily. The opening of the file and any resulting exceptions occur + /// upon enumeration. + /// + /// is . Thrown eagerly. + /// Client is not connected upon enumeration. + /// The return value is enumerated after the client is disposed. public IEnumerable ReadLines(string path) { return ReadLines(path, Encoding.UTF8); @@ -1819,33 +1823,34 @@ public IEnumerable ReadLines(string path) /// /// The lines of the file. /// - /// is . - /// Client is not connected. - /// The method was called after the client was disposed. + /// + /// The lines are enumerated lazily. The opening of the file and any resulting exceptions occur + /// upon enumeration. + /// + /// is . Thrown eagerly. + /// Client is not connected upon enumeration. + /// The return value is enumerated after the client is disposed. public IEnumerable ReadLines(string path, Encoding encoding) { - // We open the file eagerly i.e. outside of the state machine created by yield, - // in order to a) throw resulting (e.g. file-related) exceptions eagerly; and b) - // to match what File.ReadLines does. - // This probably makes it behave more predictably/closer to what most people expect. - // The downside is that if the return value is never enumerated, the file - // is never closed (we can't do "using" here because it would be disposed - // as soon as we return). This conundrum also exists with File.ReadLines. - - var sr = new StreamReader(OpenRead(path), encoding); + // We allow this usage exception to throw eagerly... + ThrowHelper.ThrowIfNull(path); - return Enumerate(sr); + // ... but other exceptions will throw lazily i.e. inside the state machine created + // by yield. We could choose to open the file eagerly as well in order to throw + // file-related exceptions eagerly (matching what File.ReadLines does), but this + // complicates double enumeration, and introduces the problem that File.ReadLines + // has whereby the file is not closed if the return value is not enumerated. + return Enumerate(); - static IEnumerable Enumerate(StreamReader sr) + IEnumerable Enumerate() { - using (sr) - { - string? line; + using var sr = new StreamReader(OpenRead(path), encoding); - while ((line = sr.ReadLine()) != null) - { - yield return line; - } + string? line; + + while ((line = sr.ReadLine()) != null) + { + yield return line; } } } diff --git a/test/Renci.SshNet.IntegrationTests/SftpTests.cs b/test/Renci.SshNet.IntegrationTests/SftpTests.cs index 062c2976d..1efc15644 100644 --- a/test/Renci.SshNet.IntegrationTests/SftpTests.cs +++ b/test/Renci.SshNet.IntegrationTests/SftpTests.cs @@ -1788,6 +1788,9 @@ public void Sftp_ReadLines_NoEncoding_ExistingFile() var actualLines = client.ReadLines(remoteFile); Assert.IsNotNull(actualLines); + + // These two lines together test double enumeration. + Assert.AreEqual(lines[0], actualLines.First()); CollectionAssert.AreEqual(lines, actualLines.ToArray()); } finally @@ -1801,7 +1804,9 @@ public void Sftp_ReadLines_NoEncoding_ExistingFile() } [TestMethod] - public void Sftp_ReadLines_NoEncoding_FileDoesNotExist() + [DataRow(false)] + [DataRow(true)] + public void Sftp_ReadLines_FileDoesNotExist(bool encoding) { using (var client = new SftpClient(_connectionInfoFactory.Create())) { @@ -1814,13 +1819,28 @@ public void Sftp_ReadLines_NoEncoding_FileDoesNotExist() client.DeleteFile(remoteFile); } + // This exception should bubble up immediately + var nullEx = encoding + ? Assert.Throws(() => client.ReadLines(null, GetRandomEncoding())) + : Assert.Throws(() => client.ReadLines(null)); + + Assert.AreEqual("path", nullEx.ParamName); + try { - client.ReadLines(remoteFile); - Assert.Fail(); - } - catch (SftpPathNotFoundException ex) - { + var ex = Assert.ThrowsExactly(() => + { + // The PathNotFound exception is permitted to bubble up only upon + // enumerating. + var lines = encoding + ? client.ReadLines(remoteFile, GetRandomEncoding()) + : client.ReadLines(remoteFile); + + using var enumerator = lines.GetEnumerator(); + + _ = enumerator.MoveNext(); + }); + Assert.IsNull(ex.InnerException); Assert.AreEqual("No such file", ex.Message); @@ -1866,46 +1886,10 @@ public void Sftp_ReadLines_Encoding_ExistingFile() var actualLines = client.ReadLines(remoteFile, encoding); Assert.IsNotNull(actualLines); - CollectionAssert.AreEqual(lines, actualLines.ToArray()); - } - finally - { - if (client.Exists(remoteFile)) - { - client.DeleteFile(remoteFile); - } - } - } - } - - [TestMethod] - public void Sftp_ReadLines_Encoding_FileDoesNotExist() - { - var encoding = GetRandomEncoding(); - - using (var client = new SftpClient(_connectionInfoFactory.Create())) - { - client.Connect(); - - var remoteFile = GenerateUniqueRemoteFileName(); - - if (client.Exists(remoteFile)) - { - client.DeleteFile(remoteFile); - } - try - { - client.ReadLines(remoteFile, encoding); - Assert.Fail(); - } - catch (SftpPathNotFoundException ex) - { - Assert.IsNull(ex.InnerException); - Assert.AreEqual("No such file", ex.Message); - - // ensure file was not created by us - Assert.IsFalse(client.Exists(remoteFile)); + // These two lines together test double enumeration. + Assert.AreEqual(lines[0], actualLines.First()); + CollectionAssert.AreEqual(lines, actualLines.ToArray()); } finally {