From f282f40ac0999037b3a72fa77d4d493831d0325f Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Sun, 24 May 2026 16:18:31 +0200 Subject: [PATCH 1/2] Use the read buffer in UploadFile for the SFTP write packets In SftpClient.UploadFile, a buffer is allocated to read from the given stream, and for each read, another array is allocated for the SFTP write packet (which consists of that data prepended with headers). This change effectively leaves space at the start of the buffer for the headers such that it can be used to assemble the packets without that per-packet array allocation. There are cleaner/more general ways to do this (e.g. for all packet types, leave space for the SSH headers as well), but this gets the most impact for about as much effort as I can be bothered with. --- src/Renci.SshNet/Sftp/ISftpSession.cs | 12 +- .../Sftp/Requests/SftpWriteRequest.cs | 99 ++++++++------ .../Sftp/Requests/SftpWriteRequestBuffer.cs | 127 ++++++++++++++++++ src/Renci.SshNet/Sftp/SftpSession.cs | 37 +++-- src/Renci.SshNet/SftpClient.cs | 15 ++- src/Renci.SshNet/SubsystemSession.cs | 13 +- .../Sftp/Requests/SftpWriteRequestTest.cs | 10 +- .../Classes/Sftp/SftpFileStreamTest.cs | 7 +- .../SftpSessionTest_Connected_RequestRead.cs | 6 +- ...ftpSessionTest_Connected_RequestStatVfs.cs | 6 +- ...tipleSftpMessagesInSingleSshDataMessage.cs | 8 +- ...essagesSplitOverMultipleSshDataMessages.cs | 8 +- ...eived_SingleSftpMessageInSshDataMessage.cs | 6 +- .../SubsystemSession_SendData_Connected.cs | 4 +- 14 files changed, 268 insertions(+), 90 deletions(-) create mode 100644 src/Renci.SshNet/Sftp/Requests/SftpWriteRequestBuffer.cs diff --git a/src/Renci.SshNet/Sftp/ISftpSession.cs b/src/Renci.SshNet/Sftp/ISftpSession.cs index 4a804a35e..0349bcd8c 100644 --- a/src/Renci.SshNet/Sftp/ISftpSession.cs +++ b/src/Renci.SshNet/Sftp/ISftpSession.cs @@ -4,6 +4,7 @@ using System.Threading.Tasks; using Renci.SshNet.Common; +using Renci.SshNet.Sftp.Requests; using Renci.SshNet.Sftp.Responses; namespace Renci.SshNet.Sftp @@ -325,14 +326,19 @@ internal interface ISftpSession : ISubsystemSession /// the zero-based offset in at which to begin taking bytes to write. /// The length (in bytes) of the data to write. /// The wait event handle if needed. - /// The callback to invoke when the write has completed. void RequestWrite(byte[] handle, ulong serverOffset, byte[] data, int offset, int length, - AutoResetEvent wait, - Action writeCompleted = null); + AutoResetEvent wait); + + /// + /// Performs SSH_FXP_WRITE request. + /// + /// The buffer. + /// The callback to invoke when the write has completed. + void RequestWrite(SftpWriteRequestBuffer buffer, Action writeCompleted); /// /// Asynchronouly performs a SSH_FXP_WRITE request. diff --git a/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs index 6627513ae..2728fe7c3 100644 --- a/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs +++ b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs @@ -1,17 +1,27 @@ using System; +using System.Buffers.Binary; +using Renci.SshNet.Common; using Renci.SshNet.Sftp.Responses; namespace Renci.SshNet.Sftp.Requests { internal sealed class SftpWriteRequest : SftpRequest { + private readonly SftpWriteRequestBuffer _buffer; + public override SftpMessageTypes SftpMessageType { get { return SftpMessageTypes.Write; } } - public byte[] Handle { get; private set; } + public ReadOnlySpan Handle + { + get + { + return _buffer.Handle; + } + } /// /// Gets the zero-based offset (in bytes) relative to the beginning of the file that the write @@ -21,7 +31,13 @@ public override SftpMessageTypes SftpMessageType /// The zero-based offset (in bytes) relative to the beginning of the file that the write must /// start at. /// - public ulong ServerFileOffset { get; private set; } + public ulong ServerFileOffset + { + get + { + return _buffer.ServerFileOffset; + } + } /// /// Gets the buffer holding the data to write. @@ -29,36 +45,19 @@ public override SftpMessageTypes SftpMessageType /// /// The buffer holding the data to write. /// - public byte[] Data { get; private set; } - - /// - /// Gets the zero-based offset in at which to begin taking bytes to - /// write. - /// - /// - /// The zero-based offset in at which to begin taking bytes to write. - /// - public int Offset { get; private set; } - - /// - /// Gets the length (in bytes) of the data to write. - /// - /// - /// The length (in bytes) of the data to write. - /// - public int Length { get; private set; } + public ReadOnlySpan Data + { + get + { + return _buffer.Data.AsSpan(0, _buffer.DataLength); + } + } protected override int BufferCapacity { get { - var capacity = base.BufferCapacity; - capacity += 4; // Handle length - capacity += Handle.Length; // Handle - capacity += 8; // ServerFileOffset length - capacity += 4; // Data length - capacity += Length; // Data - return capacity; + return _buffer.ActiveBytes.Count; } } @@ -72,31 +71,45 @@ public SftpWriteRequest(uint protocolVersion, Action statusAction) : base(protocolVersion, requestId, statusAction) { - Handle = handle; - ServerFileOffset = serverFileOffset; - Data = data; - Offset = offset; - Length = length; + _buffer = new SftpWriteRequestBuffer(handle, serverFileOffset, data.AsSpan(offset, length)) + { + RequestId = requestId, + }; } - protected override void LoadData() + public SftpWriteRequest(uint protocolVersion, + SftpWriteRequestBuffer buffer, + Action statusAction) + : base(protocolVersion, buffer.RequestId, statusAction) { - base.LoadData(); + _buffer = buffer; + } - Handle = ReadBinary(); - ServerFileOffset = ReadUInt64(); - Data = ReadBinary(); - Offset = 0; - Length = Data.Length; + protected override void LoadData() + { + throw new NotImplementedException(); } protected override void SaveData() { - base.SaveData(); + throw new NotImplementedException(); + } + + protected override void WriteBytes(SshDataStream stream) + { + var activeBuffer = GetBytes(); + + stream.Write(activeBuffer.Array, activeBuffer.Offset, activeBuffer.Count); + } + + public new ArraySegment GetBytes() + { + var activeBuffer = _buffer.ActiveBytes; + + // Write SFTP packet length. + BinaryPrimitives.WriteInt32BigEndian(activeBuffer.AsSpan(), activeBuffer.Count - 4); - WriteBinaryString(Handle); - Write(ServerFileOffset); - WriteBinary(Data, Offset, Length); + return activeBuffer; } } } diff --git a/src/Renci.SshNet/Sftp/Requests/SftpWriteRequestBuffer.cs b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequestBuffer.cs new file mode 100644 index 000000000..64dc6f2ab --- /dev/null +++ b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequestBuffer.cs @@ -0,0 +1,127 @@ +#nullable enable +using System; +using System.Buffers.Binary; +using System.Diagnostics; + +namespace Renci.SshNet.Sftp.Requests +{ + /// + /// A helper type that wraps a buffer for SFTP write requests. + /// + /// + /// [Sftp packet length, SftpMessageType, RequestId, Handle length, Handle, Server offset, data length, data]. + /// [ 4, 1, 4, 4, ?, 8, 4, ?]. + /// + internal sealed class SftpWriteRequestBuffer + { + private const int MessageTypeOffset = 4; + private const int RequestIdOffset = MessageTypeOffset + 1; + private const int HandleLengthOffset = RequestIdOffset + 4; + private const int HandleOffset = HandleLengthOffset + 4; + + private readonly byte[] _buffer; + + public ArraySegment ActiveBytes + { + get + { + return new(_buffer, 0, HandleOffset + HandleLength + 8 + 4 + DataLength); + } + } + + public SftpWriteRequestBuffer(ReadOnlySpan handle, int dataCapacity) + { + Debug.Assert(dataCapacity >= 0); + + _buffer = new byte[HandleOffset + handle.Length + 8 + 4 + dataCapacity]; + + _buffer[MessageTypeOffset] = (byte)SftpMessageTypes.Write; + + HandleLength = handle.Length; + + handle.CopyTo(_buffer.AsSpan(HandleOffset)); + } + + public SftpWriteRequestBuffer(ReadOnlySpan handle, ulong serverFileOffset, ReadOnlySpan data) + : this(handle, data.Length) + { + ServerFileOffset = serverFileOffset; + + DataLength = data.Length; + + data.CopyTo(Data); + } + + public uint RequestId + { + get + { + return BinaryPrimitives.ReadUInt32BigEndian(_buffer.AsSpan(RequestIdOffset)); + } + set + { + BinaryPrimitives.WriteUInt32BigEndian(_buffer.AsSpan(RequestIdOffset), value); + } + } + + public int HandleLength + { + get + { + return BinaryPrimitives.ReadInt32BigEndian(_buffer.AsSpan(HandleLengthOffset)); + } + private init + { + Debug.Assert(value >= 0); + BinaryPrimitives.WriteInt32BigEndian(_buffer.AsSpan(HandleLengthOffset), value); + } + } + + public ReadOnlySpan Handle + { + get + { + return _buffer.AsSpan(HandleOffset, HandleLength); + } + } + + public ulong ServerFileOffset + { + get + { + return BinaryPrimitives.ReadUInt64BigEndian(_buffer.AsSpan(HandleOffset + HandleLength)); + } + set + { + BinaryPrimitives.WriteUInt64BigEndian(_buffer.AsSpan(HandleOffset + HandleLength), value); + } + } + + public int DataLength + { + get + { + return BinaryPrimitives.ReadInt32BigEndian(_buffer.AsSpan(HandleOffset + HandleLength + 8)); + } + set + { + Debug.Assert(value >= 0); + Debug.Assert(value <= _buffer.Length - (HandleOffset + HandleLength + 8 + 4)); + + BinaryPrimitives.WriteInt32BigEndian(_buffer.AsSpan(HandleOffset + HandleLength + 8), value); + } + } + + /// + /// Gets the space available to write as file data. Does not consider . + /// + public ArraySegment Data + { + get + { + var offset = HandleOffset + HandleLength + 8 + 4; + return new ArraySegment(_buffer, offset, _buffer.Length - offset); + } + } + } +} diff --git a/src/Renci.SshNet/Sftp/SftpSession.cs b/src/Renci.SshNet/Sftp/SftpSession.cs index d5648bda4..6d3ac28b8 100644 --- a/src/Renci.SshNet/Sftp/SftpSession.cs +++ b/src/Renci.SshNet/Sftp/SftpSession.cs @@ -87,8 +87,16 @@ public async Task ChangeDirectoryAsync(string path, CancellationToken cancellati internal void SendMessage(SftpMessage sftpMessage) { - var data = sftpMessage.GetBytes(); - SendData(data); + if (sftpMessage is SftpWriteRequest writeRequest) + { + var data = writeRequest.GetBytes(); + SendData(data.Array, data.Offset, data.Count); + } + else + { + var data = sftpMessage.GetBytes(); + SendData(data); + } } /// @@ -579,20 +587,31 @@ public void RequestWrite(byte[] handle, byte[] data, int offset, int length, + AutoResetEvent wait) + { + var buffer = new SftpWriteRequestBuffer(handle, serverOffset, data.AsSpan(offset, length)); + + RequestWrite(buffer, wait, writeCompleted: null); + } + + /// + public void RequestWrite(SftpWriteRequestBuffer buffer, Action writeCompleted) + { + RequestWrite(buffer, wait: null, writeCompleted); + } + + private void RequestWrite(SftpWriteRequestBuffer buffer, AutoResetEvent wait, - Action writeCompleted = null) + Action writeCompleted) { Debug.Assert((wait is null) != (writeCompleted is null), "Should have one parameter or the other."); SftpException exception = null; + buffer.RequestId = NextRequestId; + var request = new SftpWriteRequest(ProtocolVersion, - NextRequestId, - handle, - serverOffset, - data, - offset, - length, + buffer, response => { if (writeCompleted is not null) diff --git a/src/Renci.SshNet/SftpClient.cs b/src/Renci.SshNet/SftpClient.cs index 960c6261f..b54f48dc3 100644 --- a/src/Renci.SshNet/SftpClient.cs +++ b/src/Renci.SshNet/SftpClient.cs @@ -16,6 +16,7 @@ using Renci.SshNet.Abstractions; using Renci.SshNet.Common; using Renci.SshNet.Sftp; +using Renci.SshNet.Sftp.Requests; namespace Renci.SshNet { @@ -2477,7 +2478,8 @@ private async Task InternalUploadFile( ulong offset = 0; // create buffer of optimal length - var buffer = new byte[_sftpSession.CalculateOptimalWriteLength(_bufferSize, handle)]; + var buffer = new SftpWriteRequestBuffer(handle, (int)_sftpSession.CalculateOptimalWriteLength(_bufferSize, handle)); + var dataBuffer = buffer.Data; var expectedResponses = 0; @@ -2492,11 +2494,11 @@ private async Task InternalUploadFile( { var bytesRead = isAsync #if NET - ? await input.ReadAsync(buffer, cancellationToken).ConfigureAwait(false) + ? await input.ReadAsync(dataBuffer, cancellationToken).ConfigureAwait(false) #else - ? await input.ReadAsync(buffer, 0, buffer.Length, cancellationToken).ConfigureAwait(false) + ? await input.ReadAsync(dataBuffer.Array, dataBuffer.Offset, dataBuffer.Count, cancellationToken).ConfigureAwait(false) #endif - : input.Read(buffer, 0, buffer.Length); + : input.Read(dataBuffer.Array!, dataBuffer.Offset, dataBuffer.Count); if (bytesRead == 0) { @@ -2510,12 +2512,15 @@ private async Task InternalUploadFile( exception?.Throw(); + buffer.ServerFileOffset = offset; + buffer.DataLength = bytesRead; + var writtenBytes = offset + (ulong)bytesRead; _ = Interlocked.Increment(ref expectedResponses); mres.Reset(); - _sftpSession.RequestWrite(handle, offset, buffer, offset: 0, bytesRead, wait: null, s => + _sftpSession.RequestWrite(buffer, s => { var setHandle = false; diff --git a/src/Renci.SshNet/SubsystemSession.cs b/src/Renci.SshNet/SubsystemSession.cs index 47e37161c..3185cf52c 100644 --- a/src/Renci.SshNet/SubsystemSession.cs +++ b/src/Renci.SshNet/SubsystemSession.cs @@ -165,11 +165,22 @@ public void Disconnect() /// /// The data to be sent. public void SendData(byte[] data) + { + SendData(data, 0, data.Length); + } + + /// + /// Sends data to the subsystem. + /// + /// The data to be sent. + /// The zero-based byte offset in at which to begin sending bytes. + /// The number of bytes to send. + public void SendData(byte[] data, int offset, int count) { ObjectDisposedException.ThrowIf(_isDisposed, this); EnsureSessionIsOpen(); - _channel.SendData(data); + _channel.SendData(data, offset, count); } /// diff --git a/test/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs b/test/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs index 6083455fa..c6add1075 100644 --- a/test/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs +++ b/test/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs @@ -43,10 +43,8 @@ public void Constructor() { var request = new SftpWriteRequest(_protocolVersion, _requestId, _handle, _serverFileOffset, _data, _offset, _length, null); - Assert.AreSame(_data, request.Data); - Assert.AreSame(_handle, request.Handle); - Assert.AreEqual(_length, request.Length); - Assert.AreEqual(_offset, request.Offset); + CollectionAssert.AreEqual(_data.Take(_offset, _length).ToArray(), request.Data.ToArray()); + CollectionAssert.AreEqual(_handle, request.Handle.ToArray()); Assert.AreEqual(_protocolVersion, request.ProtocolVersion); Assert.AreEqual(_requestId, request.RequestId); Assert.AreEqual(_serverFileOffset, request.ServerFileOffset); @@ -81,7 +79,7 @@ public void GetBytes() { var request = new SftpWriteRequest(_protocolVersion, _requestId, _handle, _serverFileOffset, _data, _offset, _length, null); - var bytes = request.GetBytes(); + var bytes = ((SftpRequest)request).GetBytes(); var expectedBytesLength = 0; expectedBytesLength += 4; // Length @@ -114,6 +112,8 @@ public void GetBytes() Assert.IsTrue(_data.Take(_offset, _length).SequenceEqual(actualData)); Assert.IsTrue(sshDataStream.IsEndOfData); + + CollectionAssert.AreEqual(bytes, request.GetBytes().ToArray()); } } } diff --git a/test/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest.cs b/test/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest.cs index 2d497f245..ebc87ca68 100644 --- a/test/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest.cs +++ b/test/Renci.SshNet.Tests/Classes/Sftp/SftpFileStreamTest.cs @@ -11,7 +11,6 @@ using Renci.SshNet.Common; using Renci.SshNet.Sftp; -using Renci.SshNet.Sftp.Responses; namespace Renci.SshNet.Tests.Classes.Sftp { @@ -240,8 +239,7 @@ private void TestSendsBufferedWrites(Action flushAction) It.IsAny(), It.IsAny(), It.IsAny(), - It.IsAny(), - It.IsAny>()), + It.IsAny()), Times.Never); // Whatever is called here should trigger the bytes to be sent @@ -336,8 +334,7 @@ private static void VerifyRequestWrite(Mock sessionMock, ReadOnlyM /* data: */ It.Is(x => IndexOf(x, newData) >= 0), /* offset: */ It.IsAny(), /* length: */ newData.Length, - /* wait: */ It.IsAny(), - /* writeCompleted: */ It.IsAny>()), + /* wait: */ It.IsAny()), Times.Once); } diff --git a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_Connected_RequestRead.cs b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_Connected_RequestRead.cs index b0bc6157a..48031454d 100644 --- a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_Connected_RequestRead.cs +++ b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_Connected_RequestRead.cs @@ -114,14 +114,14 @@ private void SetupMocks() _channelSessionMock.InSequence(sequence).Setup(p => p.Open()); _channelSessionMock.InSequence(sequence).Setup(p => p.SendSubsystemRequest("sftp")).Returns(true); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpInitRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpInitRequestBytes, 0, _sftpInitRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, new ChannelDataEventArgs(0, _sftpVersionResponse.GetBytes())); }); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpRealPathRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpRealPathRequestBytes, 0, _sftpRealPathRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, @@ -131,7 +131,7 @@ private void SetupMocks() #endregion SftpSession.Connect() _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpReadRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpReadRequestBytes, 0, _sftpReadRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise( diff --git a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_Connected_RequestStatVfs.cs b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_Connected_RequestStatVfs.cs index a5086d3d2..684e7b522 100644 --- a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_Connected_RequestStatVfs.cs +++ b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_Connected_RequestStatVfs.cs @@ -115,7 +115,7 @@ private void SetupMocks() .Setup(p => p.IsOpen) .Returns(true); _ = _channelSessionMock.InSequence(sequence) - .Setup(p => p.SendData(_sftpInitRequestBytes)) + .Setup(p => p.SendData(_sftpInitRequestBytes, 0, _sftpInitRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, @@ -125,7 +125,7 @@ private void SetupMocks() .Setup(p => p.IsOpen) .Returns(true); _ = _channelSessionMock.InSequence(sequence) - .Setup(p => p.SendData(_sftpRealPathRequestBytes)) + .Setup(p => p.SendData(_sftpRealPathRequestBytes, 0, _sftpRealPathRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, @@ -138,7 +138,7 @@ private void SetupMocks() .Setup(p => p.IsOpen) .Returns(true); _ = _channelSessionMock.InSequence(sequence) - .Setup(p => p.SendData(_sftpStatVfsRequestBytes)) + .Setup(p => p.SendData(_sftpStatVfsRequestBytes, 0, _sftpStatVfsRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, diff --git a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_MultipleSftpMessagesInSingleSshDataMessage.cs b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_MultipleSftpMessagesInSingleSshDataMessage.cs index feada4d09..8bcd6f636 100644 --- a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_MultipleSftpMessagesInSingleSshDataMessage.cs +++ b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_MultipleSftpMessagesInSingleSshDataMessage.cs @@ -132,7 +132,7 @@ private void SetupMocks() _channelSessionMock.InSequence(sequence).Setup(p => p.Open()); _channelSessionMock.InSequence(sequence).Setup(p => p.SendSubsystemRequest("sftp")).Returns(true); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpInitRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpInitRequestBytes, 0, _sftpInitRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, @@ -142,7 +142,7 @@ private void SetupMocks() .Setup(p => p.Create(0U, (byte)SftpMessageTypes.Version, _encoding)) .Returns(_sftpVersionResponse); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpRealPathRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpRealPathRequestBytes, 0, _sftpRealPathRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, @@ -155,9 +155,9 @@ private void SetupMocks() #endregion SftpSession.Connect() _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpOpenRequestBytes)); + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpOpenRequestBytes, 0, _sftpOpenRequestBytes.Length)); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpReadRequestBytes)).Callback(() => + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpReadRequestBytes, 0, _sftpReadRequestBytes.Length)).Callback(() => { var sshMessagePayload = new byte[_sftpHandleResponseBytes.Length + _sftpDataResponseBytes.Length]; Buffer.BlockCopy(_sftpHandleResponseBytes, 0, sshMessagePayload, 0, _sftpHandleResponseBytes.Length); diff --git a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_MultipleSftpMessagesSplitOverMultipleSshDataMessages.cs b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_MultipleSftpMessagesSplitOverMultipleSshDataMessages.cs index fa8728e6e..0937686ef 100644 --- a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_MultipleSftpMessagesSplitOverMultipleSshDataMessages.cs +++ b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_MultipleSftpMessagesSplitOverMultipleSshDataMessages.cs @@ -130,7 +130,7 @@ private void SetupMocks() _channelSessionMock.InSequence(sequence).Setup(p => p.Open()); _channelSessionMock.InSequence(sequence).Setup(p => p.SendSubsystemRequest("sftp")).Returns(true); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpInitRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpInitRequestBytes, 0, _sftpInitRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, @@ -140,7 +140,7 @@ private void SetupMocks() .Setup(p => p.Create(0U, (byte)SftpMessageTypes.Version, _encoding)) .Returns(_sftpVersionResponse); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpRealPathRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpRealPathRequestBytes, 0, _sftpRealPathRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, @@ -153,7 +153,7 @@ private void SetupMocks() #endregion SftpSession.Connect() _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpOpenRequestBytes)).Callback(() => + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpOpenRequestBytes, 0, _sftpOpenRequestBytes.Length)).Callback(() => { var sshMessagePayload = new byte[_sftpHandleResponseBytes.Length + 40]; Buffer.BlockCopy(_sftpHandleResponseBytes, 0, sshMessagePayload, 0, _sftpHandleResponseBytes.Length); @@ -166,7 +166,7 @@ private void SetupMocks() .Setup(p => p.Create(_protocolVersion, (byte)SftpMessageTypes.Handle, _encoding)) .Returns(new SftpHandleResponse(_protocolVersion)); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpReadRequestBytes)).Callback(() => + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpReadRequestBytes, 0, _sftpReadRequestBytes.Length)).Callback(() => { var sshMessagePayload = new byte[_sftpDataResponseBytes.Length - 40]; Buffer.BlockCopy(_sftpDataResponseBytes, 40, sshMessagePayload, 0, _sftpDataResponseBytes.Length - 40); diff --git a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_SingleSftpMessageInSshDataMessage.cs b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_SingleSftpMessageInSshDataMessage.cs index f5f570e60..ae5f466b2 100644 --- a/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_SingleSftpMessageInSshDataMessage.cs +++ b/test/Renci.SshNet.Tests/Classes/Sftp/SftpSessionTest_DataReceived_SingleSftpMessageInSshDataMessage.cs @@ -113,7 +113,7 @@ private void SetupMocks() _channelSessionMock.InSequence(sequence).Setup(p => p.Open()); _channelSessionMock.InSequence(sequence).Setup(p => p.SendSubsystemRequest("sftp")).Returns(true); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpInitRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpInitRequestBytes, 0, _sftpInitRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, @@ -123,7 +123,7 @@ private void SetupMocks() .Setup(p => p.Create(0U, (byte)SftpMessageTypes.Version, _encoding)) .Returns(_sftpVersionResponse); _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpRealPathRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpRealPathRequestBytes, 0, _sftpRealPathRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, @@ -136,7 +136,7 @@ private void SetupMocks() #endregion SftpSession.Connect() _channelSessionMock.InSequence(sequence).Setup(p => p.IsOpen).Returns(true); - _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpReadRequestBytes)) + _channelSessionMock.InSequence(sequence).Setup(p => p.SendData(_sftpReadRequestBytes, 0, _sftpReadRequestBytes.Length)) .Callback(() => { _channelSessionMock.Raise(c => c.DataReceived += null, diff --git a/test/Renci.SshNet.Tests/Classes/SubsystemSession_SendData_Connected.cs b/test/Renci.SshNet.Tests/Classes/SubsystemSession_SendData_Connected.cs index eb19eac14..4cb1c9565 100644 --- a/test/Renci.SshNet.Tests/Classes/SubsystemSession_SendData_Connected.cs +++ b/test/Renci.SshNet.Tests/Classes/SubsystemSession_SendData_Connected.cs @@ -50,7 +50,7 @@ protected void Arrange() _channelMock.InSequence(_sequence).Setup(p => p.Open()); _channelMock.InSequence(_sequence).Setup(p => p.SendSubsystemRequest(_subsystemName)).Returns(true); _channelMock.InSequence(_sequence).Setup(p => p.IsOpen).Returns(true); - _channelMock.InSequence(_sequence).Setup(p => p.SendData(_data)); + _channelMock.InSequence(_sequence).Setup(p => p.SendData(_data, 0, _data.Length)); _subsystemSession = new SubsystemSessionStub( _sessionMock.Object, @@ -81,7 +81,7 @@ public void ErrorOccurredHasNeverFired() [TestMethod] public void SendDataOnChannelShouldBeInvokedOnce() { - _channelMock.Verify(p => p.SendData(_data), Times.Once); + _channelMock.Verify(p => p.SendData(_data, 0, _data.Length), Times.Once); } [TestMethod] From f0863c8051b0fb26ea31bd8ee1fffcd12f2a2089 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Mon, 25 May 2026 20:21:07 +0200 Subject: [PATCH 2/2] Rent from pool --- .../Sftp/Requests/SftpWriteRequest.cs | 16 -------- .../Sftp/Requests/SftpWriteRequestBuffer.cs | 36 ++++++++++++++---- src/Renci.SshNet/Sftp/SftpSession.cs | 38 +++++++++---------- src/Renci.SshNet/SftpClient.cs | 13 +++++-- .../Sftp/Requests/SftpWriteRequestTest.cs | 28 +++++++++----- 5 files changed, 76 insertions(+), 55 deletions(-) diff --git a/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs index 2728fe7c3..0725d42dc 100644 --- a/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs +++ b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequest.cs @@ -61,22 +61,6 @@ protected override int BufferCapacity } } - public SftpWriteRequest(uint protocolVersion, - uint requestId, - byte[] handle, - ulong serverFileOffset, - byte[] data, - int offset, - int length, - Action statusAction) - : base(protocolVersion, requestId, statusAction) - { - _buffer = new SftpWriteRequestBuffer(handle, serverFileOffset, data.AsSpan(offset, length)) - { - RequestId = requestId, - }; - } - public SftpWriteRequest(uint protocolVersion, SftpWriteRequestBuffer buffer, Action statusAction) diff --git a/src/Renci.SshNet/Sftp/Requests/SftpWriteRequestBuffer.cs b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequestBuffer.cs index 64dc6f2ab..9f7582e94 100644 --- a/src/Renci.SshNet/Sftp/Requests/SftpWriteRequestBuffer.cs +++ b/src/Renci.SshNet/Sftp/Requests/SftpWriteRequestBuffer.cs @@ -1,5 +1,6 @@ #nullable enable -using System; +using System; +using System.Buffers; using System.Buffers.Binary; using System.Diagnostics; @@ -12,14 +13,15 @@ namespace Renci.SshNet.Sftp.Requests /// [Sftp packet length, SftpMessageType, RequestId, Handle length, Handle, Server offset, data length, data]. /// [ 4, 1, 4, 4, ?, 8, 4, ?]. /// - internal sealed class SftpWriteRequestBuffer + internal sealed class SftpWriteRequestBuffer : IDisposable { private const int MessageTypeOffset = 4; private const int RequestIdOffset = MessageTypeOffset + 1; private const int HandleLengthOffset = RequestIdOffset + 4; private const int HandleOffset = HandleLengthOffset + 4; - private readonly byte[] _buffer; + private readonly bool _usePool; + private byte[] _buffer; public ArraySegment ActiveBytes { @@ -29,11 +31,17 @@ public ArraySegment ActiveBytes } } - public SftpWriteRequestBuffer(ReadOnlySpan handle, int dataCapacity) + public SftpWriteRequestBuffer(ReadOnlySpan handle, int dataCapacity, bool usePool = false) { Debug.Assert(dataCapacity >= 0); - _buffer = new byte[HandleOffset + handle.Length + 8 + 4 + dataCapacity]; + var totalCapacity = HandleOffset + handle.Length + 8 + 4 + dataCapacity; + + _usePool = usePool; + + _buffer = usePool + ? ArrayPool.Shared.Rent(totalCapacity) + : new byte[totalCapacity]; _buffer[MessageTypeOffset] = (byte)SftpMessageTypes.Write; @@ -42,8 +50,8 @@ public SftpWriteRequestBuffer(ReadOnlySpan handle, int dataCapacity) handle.CopyTo(_buffer.AsSpan(HandleOffset)); } - public SftpWriteRequestBuffer(ReadOnlySpan handle, ulong serverFileOffset, ReadOnlySpan data) - : this(handle, data.Length) + public SftpWriteRequestBuffer(ReadOnlySpan handle, ulong serverFileOffset, ReadOnlySpan data, bool usePool = false) + : this(handle, data.Length, usePool) { ServerFileOffset = serverFileOffset; @@ -123,5 +131,19 @@ public ArraySegment Data return new ArraySegment(_buffer, offset, _buffer.Length - offset); } } + + public void Dispose() + { + if (_usePool) + { + var buffer = _buffer; + _buffer = null!; + + if (buffer is not null) + { + ArrayPool.Shared.Return(buffer); + } + } + } } } diff --git a/src/Renci.SshNet/Sftp/SftpSession.cs b/src/Renci.SshNet/Sftp/SftpSession.cs index 6d3ac28b8..6b2198053 100644 --- a/src/Renci.SshNet/Sftp/SftpSession.cs +++ b/src/Renci.SshNet/Sftp/SftpSession.cs @@ -589,7 +589,7 @@ public void RequestWrite(byte[] handle, int length, AutoResetEvent wait) { - var buffer = new SftpWriteRequestBuffer(handle, serverOffset, data.AsSpan(offset, length)); + using var buffer = new SftpWriteRequestBuffer(handle, serverOffset, data.AsSpan(offset, length), usePool: true); RequestWrite(buffer, wait, writeCompleted: null); } @@ -648,24 +648,24 @@ public Task RequestWriteAsync(byte[] handle, ulong serverOffset, byte[] data, in var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - SendRequest(new SftpWriteRequest(ProtocolVersion, - NextRequestId, - handle, - serverOffset, - data, - offset, - length, - response => - { - if (response.StatusCode == StatusCode.Ok) - { - _ = tcs.TrySetResult(true); - } - else - { - _ = tcs.TrySetException(GetSftpException(response)); - } - })); + using (var buffer = new SftpWriteRequestBuffer(handle, serverOffset, data.AsSpan(offset, length), usePool: true)) + { + buffer.RequestId = NextRequestId; + + SendRequest(new SftpWriteRequest(ProtocolVersion, + buffer, + response => + { + if (response.StatusCode == StatusCode.Ok) + { + _ = tcs.TrySetResult(true); + } + else + { + _ = tcs.TrySetException(GetSftpException(response)); + } + })); + } return WaitOnHandleAsync(tcs, OperationTimeout, cancellationToken); } diff --git a/src/Renci.SshNet/SftpClient.cs b/src/Renci.SshNet/SftpClient.cs index b54f48dc3..f8d483d2a 100644 --- a/src/Renci.SshNet/SftpClient.cs +++ b/src/Renci.SshNet/SftpClient.cs @@ -2478,9 +2478,14 @@ private async Task InternalUploadFile( ulong offset = 0; // create buffer of optimal length - var buffer = new SftpWriteRequestBuffer(handle, (int)_sftpSession.CalculateOptimalWriteLength(_bufferSize, handle)); + var dataCapacity = (int)_sftpSession.CalculateOptimalWriteLength(_bufferSize, handle); + + using var buffer = new SftpWriteRequestBuffer(handle, dataCapacity, usePool: true); + var dataBuffer = buffer.Data; + Debug.Assert(dataBuffer.Count >= dataCapacity); + var expectedResponses = 0; // We will send out all the write requests without waiting for each response. @@ -2494,11 +2499,11 @@ private async Task InternalUploadFile( { var bytesRead = isAsync #if NET - ? await input.ReadAsync(dataBuffer, cancellationToken).ConfigureAwait(false) + ? await input.ReadAsync(dataBuffer.AsMemory(0, dataCapacity), cancellationToken).ConfigureAwait(false) #else - ? await input.ReadAsync(dataBuffer.Array, dataBuffer.Offset, dataBuffer.Count, cancellationToken).ConfigureAwait(false) + ? await input.ReadAsync(dataBuffer.Array, dataBuffer.Offset, dataCapacity, cancellationToken).ConfigureAwait(false) #endif - : input.Read(dataBuffer.Array!, dataBuffer.Offset, dataBuffer.Count); + : input.Read(dataBuffer.Array!, dataBuffer.Offset, dataCapacity); if (bytesRead == 0) { diff --git a/test/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs b/test/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs index c6add1075..3ff972826 100644 --- a/test/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs +++ b/test/Renci.SshNet.Tests/Classes/Sftp/Requests/SftpWriteRequestTest.cs @@ -41,9 +41,15 @@ public void Init() [TestMethod] public void Constructor() { - var request = new SftpWriteRequest(_protocolVersion, _requestId, _handle, _serverFileOffset, _data, _offset, _length, null); + var request = new SftpWriteRequest( + _protocolVersion, + new SftpWriteRequestBuffer(_handle, _serverFileOffset, _data.AsSpan(_offset, _length)) + { + RequestId = _requestId + }, + statusAction: null); - CollectionAssert.AreEqual(_data.Take(_offset, _length).ToArray(), request.Data.ToArray()); + CollectionAssert.AreEqual(_data.Take(_offset, _length), request.Data.ToArray()); CollectionAssert.AreEqual(_handle, request.Handle.ToArray()); Assert.AreEqual(_protocolVersion, request.ProtocolVersion); Assert.AreEqual(_requestId, request.RequestId); @@ -60,12 +66,10 @@ public void Complete_SftpStatusResponse() var request = new SftpWriteRequest( _protocolVersion, - _requestId, - _handle, - _serverFileOffset, - _data, - _offset, - _length, + new SftpWriteRequestBuffer(_handle, _serverFileOffset, _data.AsSpan(_offset, _length)) + { + RequestId = _requestId + }, statusAction); request.Complete(statusResponse); @@ -77,7 +81,13 @@ public void Complete_SftpStatusResponse() [TestMethod] public void GetBytes() { - var request = new SftpWriteRequest(_protocolVersion, _requestId, _handle, _serverFileOffset, _data, _offset, _length, null); + var request = new SftpWriteRequest( + _protocolVersion, + new SftpWriteRequestBuffer(_handle, _serverFileOffset, _data.AsSpan(_offset, _length)) + { + RequestId = _requestId + }, + statusAction: null); var bytes = ((SftpRequest)request).GetBytes();