on second thought, open the file lazily in ReadLines

Author: Rob-Hague

src/Renci.SshNet/SftpClient.cs

@@ -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);
@@ -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;
                 }
             }
         }

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<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);
 
@@ -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
                 {