From 89f378a0ab99ffc85d3618da7dcc1f3a1aa06d80 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Fri, 26 Jun 2026 19:02:09 +0200 Subject: [PATCH] Keep APM and sync UploadFile/DownloadFile callbacks on the threadpool (#1805) * Tweak internal IProgress usage for APM and sync UploadFile/DownloadFile Changes to support IProgress<> callback on UploadAsync/DownloadAsync meant wrapping the Action<> callback on existing methods in a Progress<>, which posts the callback onto the current synchronisation context rather than the threadpool. For the legacy APM methods (Begin[..]), let's just preserve their old behaviour. For the synchronous methods, posting to the synchronisation context is probably the worst choice (because if there is one, the method itself is running there). We can either revert to the threadpool as well, or take the opportunity to invoke the callback synchronously, which is a behavioural change but probably the least surprising behaviour for a synchronous method. * keep callbacks on the threadpool Actually, we could call the Download callback synchronously easily enough, but the Upload progress reports are being made on the message listener thread upon request ack. A more involved scheme could drain callbacks to fire during the read loop. For now just make it all the same behaviour as in 2025.1.0. --- src/Renci.SshNet/SftpClient.cs | 66 ++++++---- .../SftpClientTest.Upload.cs | 118 ++++++++++++------ 2 files changed, 122 insertions(+), 62 deletions(-) diff --git a/src/Renci.SshNet/SftpClient.cs b/src/Renci.SshNet/SftpClient.cs index f8d483d2..4e554abb 100644 --- a/src/Renci.SshNet/SftpClient.cs +++ b/src/Renci.SshNet/SftpClient.cs @@ -906,7 +906,7 @@ namespace Renci.SshNet if (downloadCallback != null) { - downloadProgress = new Progress(r => downloadCallback(r.TotalBytesDownloaded)); + downloadProgress = new ThreadPoolProgress(r => downloadCallback(r.TotalBytesDownloaded)); } InternalDownloadFile( @@ -935,7 +935,7 @@ namespace Renci.SshNet path, output, asyncResult: null, - downloadProgress: downloadProgress, + downloadProgress, isAsync: true, cancellationToken); } @@ -1012,7 +1012,11 @@ namespace Renci.SshNet if (downloadCallback != null) { - downloadProgress = new Progress(r => downloadCallback(r.TotalBytesDownloaded)); + // The System.Progress ctor captures the current synchronization context + // and posts the progress reports to it. For back-compat with previous + // versions which always posted the callback to the threadpool regardless of + // sync context, we use a custom IProgress impl. + downloadProgress = new ThreadPoolProgress(r => downloadCallback(r.TotalBytesDownloaded)); } var asyncResult = new SftpDownloadAsyncResult(asyncCallback, state); @@ -1090,7 +1094,7 @@ namespace Renci.SshNet if (uploadCallback != null) { - uploadProgress = new Progress(r => uploadCallback(r.TotalBytesUploaded)); + uploadProgress = new ThreadPoolProgress(r => uploadCallback(r.TotalBytesUploaded)); } InternalUploadFile( @@ -1274,7 +1278,11 @@ namespace Renci.SshNet if (uploadCallback != null) { - uploadProgress = new Progress(r => uploadCallback(r.TotalBytesUploaded)); + // The System.Progress ctor captures the current synchronization context + // and posts the progress reports to it. For back-compat with previous + // versions which always posted the callback to the threadpool regardless of + // sync context, we use a custom IProgress impl. + uploadProgress = new ThreadPoolProgress(r => uploadCallback(r.TotalBytesUploaded)); } var asyncResult = new SftpUploadAsyncResult(asyncCallback, state); @@ -2418,16 +2426,10 @@ namespace Renci.SshNet asyncResult?.Update(totalBytesRead); - if (downloadProgress is not null) + downloadProgress?.Report(new DownloadFileProgressReport() { - // Copy offset to ensure it's not modified between now and execution of callback - var report = new DownloadFileProgressReport() - { - TotalBytesDownloaded = totalBytesRead, - }; - - downloadProgress.Report(report); - } + TotalBytesDownloaded = totalBytesRead + }); } } finally @@ -2546,16 +2548,10 @@ namespace Renci.SshNet asyncResult?.Update(writtenBytes); - // Call callback to report number of bytes written - if (uploadProgress is not null) + uploadProgress?.Report(new UploadFileProgressReport() { - UploadFileProgressReport report = new() - { - TotalBytesUploaded = writtenBytes, - }; - - uploadProgress.Report(report); - } + TotalBytesUploaded = writtenBytes + }); } finally { @@ -2662,5 +2658,29 @@ namespace Renci.SshNet throw; } } + + /// + /// An implementation that posts callbacks to the threadpool. + /// + private sealed class ThreadPoolProgress : IProgress + { + private readonly Action _handler; + + public ThreadPoolProgress(Action handler) + { + Debug.Assert(handler != null); + _handler = handler!; + } + + void IProgress.Report(T value) + { + _ = ThreadPool.QueueUserWorkItem(static state => + { + var (handler, value) = ((Action, T))state!; + handler(value); + }, + (_handler, value)); + } + } } } diff --git a/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs b/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs index 40c296c7..06448746 100644 --- a/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs +++ b/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs @@ -324,77 +324,104 @@ namespace Renci.SshNet.IntegrationTests.OldIntegrationTests var remoteFileName = Path.GetRandomFileName(); var localFileName = Path.GetRandomFileName(); - var uploadDelegateCalled = false; - var downloadDelegateCalled = false; - var listDirectoryDelegateCalled = false; + using var uploadDelegateEvent = new ManualResetEventSlim(); + using var downloadDelegateEvent = new ManualResetEventSlim(); + using var listDirectoryDelegateEvent = new ManualResetEventSlim(); + using var uploadCallbackEvent = new ManualResetEventSlim(); + using var downloadCallbackEvent = new ManualResetEventSlim(); + using var listDirectoryCallbackEvent = new ManualResetEventSlim(); IAsyncResult asyncResult; // Test for BeginUploadFile. CreateTestFile(localFileName, 1); - using (var fileStream = File.OpenRead(localFileName)) + var originalContext = SynchronizationContext.Current; + try { - asyncResult = sftp.BeginUploadFile(fileStream, - remoteFileName, - delegate (IAsyncResult ar) - { - sftp.EndUploadFile(ar); - uploadDelegateCalled = true; - }, - null); + // Set a throwing context to verify it's not captured by the callback + SynchronizationContext.SetSynchronizationContext(new ThrowingSynchronizationContext()); - while (!asyncResult.IsCompleted) + using (var fileStream = File.OpenRead(localFileName)) { - Thread.Sleep(500); + asyncResult = sftp.BeginUploadFile(fileStream, + remoteFileName, + delegate (IAsyncResult ar) + { + uploadDelegateEvent.Set(); + }, + state: null, + uploadCallback: _ => uploadCallbackEvent.Set()); + + sftp.EndUploadFile(asyncResult); } } + finally + { + SynchronizationContext.SetSynchronizationContext(originalContext); + } File.Delete(localFileName); - Assert.IsTrue(uploadDelegateCalled, "BeginUploadFile"); + Assert.IsTrue(uploadDelegateEvent.Wait(1000)); + Assert.IsTrue(uploadCallbackEvent.Wait(1000)); // Test for BeginDownloadFile. asyncResult = null; - using (var fileStream = File.OpenWrite(localFileName)) + try { - asyncResult = sftp.BeginDownloadFile(remoteFileName, - fileStream, - delegate (IAsyncResult ar) - { - sftp.EndDownloadFile(ar); - downloadDelegateCalled = true; - }, - null); + // Set a throwing context to verify it's not captured by the callback + SynchronizationContext.SetSynchronizationContext(new ThrowingSynchronizationContext()); - while (!asyncResult.IsCompleted) + using (var fileStream = File.OpenWrite(localFileName)) { - Thread.Sleep(500); + asyncResult = sftp.BeginDownloadFile(remoteFileName, + fileStream, + delegate (IAsyncResult ar) + { + downloadDelegateEvent.Set(); + }, + state: null, + downloadCallback: _ => downloadCallbackEvent.Set()); + + sftp.EndDownloadFile(asyncResult); } } + finally + { + SynchronizationContext.SetSynchronizationContext(originalContext); + } File.Delete(localFileName); - Assert.IsTrue(downloadDelegateCalled, "BeginDownloadFile"); + Assert.IsTrue(downloadDelegateEvent.Wait(1000)); + Assert.IsTrue(downloadCallbackEvent.Wait(1000)); // Test for BeginListDirectory. - asyncResult = null; - asyncResult = sftp.BeginListDirectory(sftp.WorkingDirectory, - delegate (IAsyncResult ar) - { - _ = sftp.EndListDirectory(ar); - listDirectoryDelegateCalled = true; - }, - null); - - while (!asyncResult.IsCompleted) + try { - Thread.Sleep(500); + // Set a throwing context to verify it's not captured by the callback + SynchronizationContext.SetSynchronizationContext(new ThrowingSynchronizationContext()); + + asyncResult = sftp.BeginListDirectory(sftp.WorkingDirectory, + delegate (IAsyncResult ar) + { + listDirectoryDelegateEvent.Set(); + }, + state: null, + listCallback: _ => listDirectoryCallbackEvent.Set()); + + _ = sftp.EndListDirectory(asyncResult); + } + finally + { + SynchronizationContext.SetSynchronizationContext(originalContext); } - Assert.IsTrue(listDirectoryDelegateCalled, "BeginListDirectory"); + Assert.IsTrue(listDirectoryDelegateEvent.Wait(1000)); + Assert.IsTrue(listDirectoryCallbackEvent.Wait(1000)); } } @@ -482,5 +509,18 @@ namespace Renci.SshNet.IntegrationTests.OldIntegrationTests Assert.IsTrue(callbackCalled); } } + + private sealed class ThrowingSynchronizationContext : SynchronizationContext + { + public override void Post(SendOrPostCallback d, object state) + { + throw new InvalidOperationException(); + } + + public override void Send(SendOrPostCallback d, object state) + { + throw new InvalidOperationException(); + } + } } }