Remove unnecessary SftpFileStream unit tests and dedup Open{Async} (#1680)

* deletions

* test additions/tweaks

* dedup SftpFileStream init logic

Author: Rob-Hague

.editorconfig

@@ -719,7 +719,7 @@ dotnet_code_quality.CA1828.api_surface = all
 dotnet_diagnostic.CA1852.severity = none
 
 # CA1848: don't enforce LoggerMessage pattern
-dotnet_diagnostic.CA1848.severity = suggestion
+dotnet_diagnostic.CA1848.severity = silent
 
 # CA1859: Change return type for improved performance
 #

src/Renci.SshNet/Common/Extensions.cs

@@ -1,12 +1,10 @@
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.Globalization;
 using System.Net;
 using System.Net.Sockets;
 using System.Numerics;
 using System.Runtime.CompilerServices;
-using System.Text;
 using System.Threading;
 
 using Renci.SshNet.Abstractions;
@@ -153,22 +151,6 @@ public static void SetIgnoringObjectDisposed(this EventWaitHandle waitHandle)
             }
         }
 
-        /// <summary>
-        /// Prints out the specified bytes.
-        /// </summary>
-        /// <param name="bytes">The bytes.</param>
-        internal static void DebugPrint(this IEnumerable<byte> bytes)
-        {
-            var sb = new StringBuilder();
-
-            foreach (var b in bytes)
-            {
-                _ = sb.AppendFormat(CultureInfo.CurrentCulture, "0x{0:x2}, ", b);
-            }
-
-            Debug.WriteLine(sb.ToString());
-        }
-
         internal static void ValidatePort(this uint value, [CallerArgumentExpression(nameof(value))] string argument = null)
         {
             if (value > IPEndPoint.MaxPort)

src/Renci.SshNet/ISubsystemSession.cs

@@ -84,57 +84,11 @@ internal interface ISubsystemSession : IDisposable
         /// <returns>A <see cref="Task"/> representing the wait.</returns>
         Task<T> WaitOnHandleAsync<T>(TaskCompletionSource<T> tcs, int millisecondsTimeout, CancellationToken cancellationToken);
 
-        /// <summary>
-        /// Blocks the current thread until the specified <see cref="WaitHandle"/> gets signaled, using a
-        /// 32-bit signed integer to specify the time interval in milliseconds.
-        /// </summary>
-        /// <param name="waitHandle">The handle to wait for.</param>
-        /// <param name="millisecondsTimeout">To number of milliseconds to wait for <paramref name="waitHandle"/> to get signaled, or <c>-1</c> to wait indefinitely.</param>
-        /// <returns>
-        /// <see langword="true"/> if <paramref name="waitHandle"/> received a signal within the specified timeout;
-        /// otherwise, <see langword="false"/>.
-        /// </returns>
-        /// <exception cref="SshException">The connection was closed by the server.</exception>
-        /// <exception cref="SshException">The channel was closed.</exception>
-        /// <remarks>
-        /// The blocking wait is also interrupted when either the established channel is closed, the current
-        /// session is disconnected or an unexpected <see cref="Exception"/> occurred while processing a channel
-        /// or session event.
-        /// </remarks>
-        bool WaitOne(WaitHandle waitHandle, int millisecondsTimeout);
-
-        /// <summary>
-        /// Blocks the current thread until the specified <see cref="WaitHandle"/> gets signaled, using a
-        /// 32-bit signed integer to specify the time interval in milliseconds.
-        /// </summary>
-        /// <param name="waitHandleA">The first handle to wait for.</param>
-        /// <param name="waitHandleB">The second handle to wait for.</param>
-        /// <param name="millisecondsTimeout">To number of milliseconds to wait for a <see cref="WaitHandle"/> to get signaled, or <c>-1</c> to wait indefinitely.</param>
-        /// <returns>
-        /// <c>0</c> if <paramref name="waitHandleA"/> received a signal within the specified timeout and <c>1</c>
-        /// if <paramref name="waitHandleB"/> received a signal within the specified timeout, or <see cref="WaitHandle.WaitTimeout"/>
-        /// if no object satisfied the wait.
-        /// </returns>
-        /// <exception cref="SshException">The connection was closed by the server.</exception>
-        /// <exception cref="SshException">The channel was closed.</exception>
-        /// <remarks>
-        /// <para>
-        /// The blocking wait is also interrupted when either the established channel is closed, the current
-        /// session is disconnected or an unexpected <see cref="Exception"/> occurred while processing a channel
-        /// or session event.
-        /// </para>
-        /// <para>
-        /// When both <paramref name="waitHandleA"/> and <paramref name="waitHandleB"/> are signaled during the call,
-        /// then <c>0</c> is returned.
-        /// </para>
-        /// </remarks>
-        int WaitAny(WaitHandle waitHandleA, WaitHandle waitHandleB, int millisecondsTimeout);
-
         /// <summary>
         /// Waits for any of the elements in the specified array to receive a signal, using a 32-bit signed
         /// integer to specify the time interval.
         /// </summary>
-        /// <param name="waitHandles">A <see cref="WaitHandle"/> array - constructed using <see cref="CreateWaitHandleArray(WaitHandle[])"/> - containing the objects to wait for.</param>
+        /// <param name="waitHandles">A <see cref="WaitHandle"/> array - constructed using <see cref="CreateWaitHandleArray"/> - containing the objects to wait for.</param>
         /// <param name="millisecondsTimeout">To number of milliseconds to wait for a <see cref="WaitHandle"/> to get signaled, or <c>-1</c> to wait indefinitely.</param>
         /// <returns>
         /// The array index of the first non-system object that satisfied the wait.
@@ -147,16 +101,6 @@ internal interface ISubsystemSession : IDisposable
         /// </remarks>
         int WaitAny(WaitHandle[] waitHandles, int millisecondsTimeout);
 
-        /// <summary>
-        /// Creates a <see cref="WaitHandle"/> array that is composed of system objects and the specified
-        /// elements.
-        /// </summary>
-        /// <param name="waitHandles">A <see cref="WaitHandle"/> array containing the objects to wait for.</param>
-        /// <returns>
-        /// A <see cref="WaitHandle"/> array that is composed of system objects and the specified elements.
-        /// </returns>
-        WaitHandle[] CreateWaitHandleArray(params WaitHandle[] waitHandles);
-
         /// <summary>
         /// Creates a <see cref="WaitHandle"/> array that is composed of system objects and the specified
         /// elements.

src/Renci.SshNet/Sftp/ISftpSession.cs

@@ -40,7 +40,7 @@ internal interface ISftpSession : ISubsystemSession
         /// <param name="path">The new working directory.</param>
         /// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
         /// <returns>A <see cref="Task"/> that tracks the asynchronous change working directory request.</returns>
-        Task ChangeDirectoryAsync(string path, CancellationToken cancellationToken = default);
+        Task ChangeDirectoryAsync(string path, CancellationToken cancellationToken);
 
         /// <summary>
         /// Resolves a given path into an absolute path on the server.
@@ -168,7 +168,7 @@ internal interface ISftpSession : ISubsystemSession
         /// <param name="path">The path.</param>
         /// <param name="cancellationToken">The <see cref="CancellationToken"/> to observe.</param>
         /// <returns>A <see cref="Task"/> that represents the asynchronous <c>SSH_FXP_MKDIR</c> operation.</returns>
-        Task RequestMkDirAsync(string path, CancellationToken cancellationToken = default);
+        Task RequestMkDirAsync(string path, CancellationToken cancellationToken);
 
         /// <summary>
         /// Performs a <c>SSH_FXP_OPEN</c> request.
@@ -389,7 +389,7 @@ internal interface ISftpSession : ISubsystemSession
         /// <returns>
         /// A task that represents the asynchronous <c>SSH_FXP_RMDIR</c> request.
         /// </returns>
-        Task RequestRmDirAsync(string path, CancellationToken cancellationToken = default);
+        Task RequestRmDirAsync(string path, CancellationToken cancellationToken);
 
         /// <summary>
         /// Performs SSH_FXP_SETSTAT request.

src/Renci.SshNet/Sftp/SftpFileStream.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.IO;
@@ -191,7 +192,7 @@ public TimeSpan Timeout
             }
         }
 
-        private SftpFileStream(ISftpSession session, string path, FileAccess access, int bufferSize, byte[] handle, long position)
+        private SftpFileStream(ISftpSession session, string path, FileAccess access, int readBufferSize, int writeBufferSize, byte[] handle, long position)
         {
             Timeout = TimeSpan.FromSeconds(30);
             Name = path;
@@ -202,25 +203,24 @@ private SftpFileStream(ISftpSession session, string path, FileAccess access, int
             _canWrite = (access & FileAccess.Write) == FileAccess.Write;
 
             _handle = handle;
+            _readBufferSize = readBufferSize;
+            _writeBufferSize = writeBufferSize;
+            _position = position;
+        }
 
-            /*
-             * Instead of using the specified buffer size as is, we use it to calculate a buffer size
-             * that ensures we always receive or send the max. number of bytes in a single SSH_FXP_READ
-             * or SSH_FXP_WRITE message.
-             */
-
-            _readBufferSize = (int)session.CalculateOptimalReadLength((uint)bufferSize);
-            _writeBufferSize = (int)session.CalculateOptimalWriteLength((uint)bufferSize, _handle);
+        internal static SftpFileStream Open(ISftpSession session, string path, FileMode mode, FileAccess access, int bufferSize)
+        {
+            return Open(session, path, mode, access, bufferSize, isAsync: false, CancellationToken.None).GetAwaiter().GetResult();
+        }
 
-            _position = position;
+        internal static Task<SftpFileStream> OpenAsync(ISftpSession session, string path, FileMode mode, FileAccess access, int bufferSize, CancellationToken cancellationToken)
+        {
+            return Open(session, path, mode, access, bufferSize, isAsync: true, cancellationToken);
         }
 
-        internal SftpFileStream(ISftpSession session, string path, FileMode mode, FileAccess access, int bufferSize)
+        private static async Task<SftpFileStream> Open(ISftpSession session, string path, FileMode mode, FileAccess access, int bufferSize, bool isAsync, CancellationToken cancellationToken)
         {
-            if (session is null)
-            {
-                throw new SshConnectionException("Client not connected.");
-            }
+            Debug.Assert(isAsync || cancellationToken == default);
 
             ThrowHelper.ThrowIfNull(path);
 
@@ -229,14 +229,10 @@ internal SftpFileStream(ISftpSession session, string path, FileMode mode, FileAc
                 throw new ArgumentOutOfRangeException(nameof(bufferSize), "Cannot be less than or equal to zero.");
             }
 
-            Timeout = TimeSpan.FromSeconds(30);
-            Name = path;
-
-            // Initialize the object state.
-            _session = session;
-            _canRead = (access & FileAccess.Read) == FileAccess.Read;
-            _canSeek = true;
-            _canWrite = (access & FileAccess.Write) == FileAccess.Write;
+            if (session is null)
+            {
+                throw new SshConnectionException("Client not connected.");
+            }
 
             var flags = Flags.None;
 
@@ -284,16 +280,7 @@ internal SftpFileStream(ISftpSession session, string path, FileMode mode, FileAc
                     flags |= Flags.Append | Flags.CreateNewOrOpen;
                     break;
                 case FileMode.Create:
-                    _handle = _session.RequestOpen(path, flags | Flags.Truncate, nullOnError: true);
-                    if (_handle is null)
-                    {
-                        flags |= Flags.CreateNew;
-                    }
-                    else
-                    {
-                        flags |= Flags.Truncate;
-                    }
-
+                    flags |= Flags.CreateNewOrOpen | Flags.Truncate;
                     break;
                 case FileMode.CreateNew:
                     flags |= Flags.CreateNew;
@@ -310,127 +297,44 @@ internal SftpFileStream(ISftpSession session, string path, FileMode mode, FileAc
                     throw new ArgumentOutOfRangeException(nameof(mode));
             }
 
-            _handle ??= _session.RequestOpen(path, flags);
+            byte[] handle;
+
+            if (isAsync)
+            {
+                handle = await session.RequestOpenAsync(path, flags, cancellationToken).ConfigureAwait(false);
+            }
+            else
+            {
+                handle = session.RequestOpen(path, flags);
+            }
 
             /*
              * Instead of using the specified buffer size as is, we use it to calculate a buffer size
              * that ensures we always receive or send the max. number of bytes in a single SSH_FXP_READ
              * or SSH_FXP_WRITE message.
              */
 
-            _readBufferSize = (int)session.CalculateOptimalReadLength((uint)bufferSize);
-            _writeBufferSize = (int)session.CalculateOptimalWriteLength((uint)bufferSize, _handle);
+            var readBufferSize = (int)session.CalculateOptimalReadLength((uint)bufferSize);
+            var writeBufferSize = (int)session.CalculateOptimalWriteLength((uint)bufferSize, handle);
 
+            long position = 0;
             if (mode == FileMode.Append)
             {
-                var attributes = _session.RequestFStat(_handle, nullOnError: false);
-                _position = attributes.Size;
-            }
-        }
-
-        internal static async Task<SftpFileStream> OpenAsync(ISftpSession session, string path, FileMode mode, FileAccess access, int bufferSize, CancellationToken cancellationToken)
-        {
-            if (session is null)
-            {
-                throw new SshConnectionException("Client not connected.");
-            }
-
-            ThrowHelper.ThrowIfNull(path);
-
-            if (bufferSize <= 0)
-            {
-                throw new ArgumentOutOfRangeException(nameof(bufferSize), "Cannot be less than or equal to zero.");
-            }
-
-            var flags = Flags.None;
-
-            switch (access)
-            {
-                case FileAccess.Read:
-                    flags |= Flags.Read;
-                    break;
-                case FileAccess.Write:
-                    flags |= Flags.Write;
-                    break;
-                case FileAccess.ReadWrite:
-                    flags |= Flags.Read;
-                    flags |= Flags.Write;
-                    break;
-                default:
-                    throw new ArgumentOutOfRangeException(nameof(access));
-            }
-
-            if ((access & FileAccess.Read) == FileAccess.Read && mode == FileMode.Append)
-            {
-                throw new ArgumentException(string.Format(CultureInfo.InvariantCulture,
-                                                          "{0} mode can be requested only when combined with write-only access.",
-                                                          mode.ToString("G")),
-                                            nameof(mode));
-            }
+                SftpFileAttributes attributes;
 
-            if ((access & FileAccess.Write) != FileAccess.Write)
-            {
-                if (mode is FileMode.Create or FileMode.CreateNew or FileMode.Truncate or FileMode.Append)
+                if (isAsync)
                 {
-                    throw new ArgumentException(string.Format(CultureInfo.InvariantCulture,
-                                                              "Combining {0}: {1} with {2}: {3} is invalid.",
-                                                              nameof(FileMode),
-                                                              mode,
-                                                              nameof(FileAccess),
-                                                              access),
-                                                nameof(mode));
+                    attributes = await session.RequestFStatAsync(handle, cancellationToken).ConfigureAwait(false);
                 }
-            }
-
-            switch (mode)
-            {
-                case FileMode.Append:
-                    flags |= Flags.Append | Flags.CreateNewOrOpen;
-                    break;
-                case FileMode.Create:
-                    flags |= Flags.CreateNewOrOpen | Flags.Truncate;
-                    break;
-                case FileMode.CreateNew:
-                    flags |= Flags.CreateNew;
-                    break;
-                case FileMode.Open:
-                    break;
-                case FileMode.OpenOrCreate:
-                    flags |= Flags.CreateNewOrOpen;
-                    break;
-                case FileMode.Truncate:
-                    flags |= Flags.Truncate;
-                    break;
-                default:
-                    throw new ArgumentOutOfRangeException(nameof(mode));
-            }
-
-            var handle = await session.RequestOpenAsync(path, flags, cancellationToken).ConfigureAwait(false);
-
-            long position = 0;
-            if (mode == FileMode.Append)
-            {
-                try
+                else
                 {
-                    var attributes = await session.RequestFStatAsync(handle, cancellationToken).ConfigureAwait(false);
-                    position = attributes.Size;
+                    attributes = session.RequestFStat(handle, nullOnError: false);
                 }
-                catch
-                {
-                    try
-                    {
-                        await session.RequestCloseAsync(handle, cancellationToken).ConfigureAwait(false);
-                    }
-                    catch
-                    {
-                        // The original exception is presumably more informative, so we just ignore this one.
-                    }
 
-                    throw;
-                }
+                position = attributes.Size;
             }
 
-            return new SftpFileStream(session, path, access, bufferSize, handle, position);
+            return new SftpFileStream(session, path, access, readBufferSize, writeBufferSize, handle, position);
         }
 
         /// <summary>