Skip to content

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

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
merged 4 commits into from
Aug 2, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f7ad83
deletions
Rob-Hague Aug 1, 2025
fb80bc6
test additions/tweaks
Rob-Hague Aug 1, 2025
e724921
dedup SftpFileStream init logic
Rob-Hague Aug 1, 2025
5107f45
Merge branch 'develop' into sftpfilestreamtests2
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
2 changes: 1 addition & 1 deletion .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
#
Expand Down
18 changes: 0 additions & 18 deletions src/Renci.SshNet/Common/Extensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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)
Expand Down
58 changes: 1 addition & 57 deletions src/Renci.SshNet/ISubsystemSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions src/Renci.SshNet/Sftp/ISftpSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
176 changes: 40 additions & 136 deletions src/Renci.SshNet/Sftp/SftpFileStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
Expand Down Expand Up @@ -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;
Expand All @@ -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);

Expand All @@ -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;

Expand Down Expand Up @@ -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;
}
Comment on lines -287 to -295
Copy link
Collaborator Author

@Rob-Hague Rob-Hague Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is at best ineffectual and at worst a bug that we send SSH_FXF_TRUNC without SSH_FXF_CREAT (what we have as Flags.CreateNewOrOpen). Per the draft SFTPv3 spec (https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-02#section-6.3), SSH_FXF_TRUNC must be accompanied by SSH_FXF_CREAT. OpenAsync had the logic correct, so we use that one. I am wondering whether it has something to do with #1215


flags |= Flags.CreateNewOrOpen | Flags.Truncate;
break;
case FileMode.CreateNew:
flags |= Flags.CreateNew;
Expand All @@ -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.
}
Comment on lines -413 to -427
Copy link
Collaborator Author

@Rob-Hague Rob-Hague Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only other library change aside from my comment above is that we use the Open (not OpenAsync) version of the FileMode.Append handling, which doesn't try RequestClose on FStat failure. Seems sensible to call it, but given the sync method is probably used much more and there are seemingly no known issues (with this part), I forwent the RequestClose


throw;
}
position = attributes.Size;
}

return new SftpFileStream(session, path, access, bufferSize, handle, position);
return new SftpFileStream(session, path, access, readBufferSize, writeBufferSize, handle, position);
}

/// <summary>
Expand Down
Loading