Skip to content

Use ReadExactly in ReadAllBytes and yield in ReadLines #1681

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.

Sign up for GitHub

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

Merged
Rob-Hague merged 3 commits into from
Aug 2, 2025
Merged

Use ReadExactly in ReadAllBytes and yield in ReadLines #1681

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3bb926
Use ReadExactly in ReadAllBytes and yield in ReadLines
Rob-Hague Aug 1, 2025
0305f88
Merge branch 'develop' into readallbytes
Rob-Hague Aug 2, 2025
501e340
on second thought, open the file lazily in ReadLines
Rob-Hague Aug 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
on second thought, open the file lazily in ReadLines
  • Loading branch information
@Rob-Hague
Rob-Hague committed Aug 2, 2025
commit 501e3400acfb18707e8d50e980c5391b978f441f
53 changes: 29 additions & 24 deletions src/Renci.SshNet/SftpClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1803,9 +1803,13 @@ public string ReadAllText(string path, Encoding encoding)
/// <returns>
/// The lines of the file.
/// </returns>
/// <exception cref="ArgumentNullException"><paramref name="path"/> is <see langword="null"/>.</exception>
/// <exception cref="SshConnectionException">Client is not connected.</exception>
/// <exception cref="ObjectDisposedException">The method was called after the client was disposed.</exception>
/// <remarks>
/// The lines are enumerated lazily. The opening of the file and any resulting exceptions occur
/// upon enumeration.
/// </remarks>
/// <exception cref="ArgumentNullException"><paramref name="path"/> is <see langword="null"/>. Thrown eagerly.</exception>
/// <exception cref="SshConnectionException">Client is not connected upon enumeration.</exception>
/// <exception cref="ObjectDisposedException">The return value is enumerated after the client is disposed.</exception>
public IEnumerable<string> ReadLines(string path)
{
return ReadLines(path, Encoding.UTF8);
Expand All @@ -1819,33 +1823,34 @@ public IEnumerable<string> ReadLines(string path)
/// <returns>
/// The lines of the file.
/// </returns>
/// <exception cref="ArgumentNullException"><paramref name="path"/> is <see langword="null"/>.</exception>
/// <exception cref="SshConnectionException">Client is not connected.</exception>
/// <exception cref="ObjectDisposedException">The method was called after the client was disposed.</exception>
/// <remarks>
/// The lines are enumerated lazily. The opening of the file and any resulting exceptions occur
/// upon enumeration.
/// </remarks>
/// <exception cref="ArgumentNullException"><paramref name="path"/> is <see langword="null"/>. Thrown eagerly.</exception>
/// <exception cref="SshConnectionException">Client is not connected upon enumeration.</exception>
/// <exception cref="ObjectDisposedException">The return value is enumerated after the client is disposed.</exception>
public IEnumerable<string> 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<string> Enumerate(StreamReader sr)
IEnumerable<string> 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;
}
}
}
Expand Down
74 changes: 29 additions & 45 deletions test/Renci.SshNet.IntegrationTests/SftpTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()))
{
Expand All @@ -1814,13 +1819,28 @@ public void Sftp_ReadLines_NoEncoding_FileDoesNotExist()
client.DeleteFile(remoteFile);
}

// This exception should bubble up immediately
var nullEx = encoding
? Assert.Throws<ArgumentNullException>(() => client.ReadLines(null, GetRandomEncoding()))
: Assert.Throws<ArgumentNullException>(() => client.ReadLines(null));

Assert.AreEqual("path", nullEx.ParamName);

try
{
client.ReadLines(remoteFile);
Assert.Fail();
}
catch (SftpPathNotFoundException ex)
{
var ex = Assert.ThrowsExactly<SftpPathNotFoundException>(() =>
{
// 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);

Expand Down Expand Up @@ -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
{
Expand Down