From 55830eb486f4bf489605704c3d573a5b93b6bd7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Pokorn=C3=BD?= Date: Thu, 21 May 2020 15:51:29 +0200 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20AsyncResult=20contains=20inv?= =?UTF-8?q?alid=20value?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AsyncResult should contain invalid value immediately after async operation is marked as completed - there was race condition problems with callback method which is invoked on different thread so updating of value is done without any synchronization. So in some cases async operation is marked as completed but async result value is not yet updated and contains invalid value --- src/Renci.SshNet/SftpClient.cs | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/Renci.SshNet/SftpClient.cs b/src/Renci.SshNet/SftpClient.cs index aa30efadb..01472dbff 100644 --- a/src/Renci.SshNet/SftpClient.cs +++ b/src/Renci.SshNet/SftpClient.cs @@ -583,7 +583,7 @@ public IEnumerable ListDirectory(string path, Action listCallbac { CheckDisposed(); - return InternalListDirectory(path, listCallback); + return InternalListDirectory(path, asyncResult: null, listCallback); } /// @@ -666,12 +666,7 @@ public IAsyncResult BeginListDirectory(string path, AsyncCallback asyncCallback, { try { - var result = InternalListDirectory(path, count => - { - asyncResult.Update(count); - - listCallback?.Invoke(count); - }); + var result = InternalListDirectory(path, asyncResult, listCallback); asyncResult.SetAsCompleted(result, completedSynchronously: false); } @@ -898,12 +893,7 @@ public IAsyncResult BeginDownloadFile(string path, Stream output, AsyncCallback { try { - InternalDownloadFile(path, output, asyncResult, offset => - { - asyncResult.Update(offset); - - downloadCallback?.Invoke(offset); - }); + InternalDownloadFile(path, output, asyncResult, downloadCallback); asyncResult.SetAsCompleted(exception: null, completedSynchronously: false); } @@ -1131,11 +1121,7 @@ public IAsyncResult BeginUploadFile(Stream input, string path, bool canOverride, { try { - InternalUploadFile(input, path, flags, asyncResult, offset => - { - asyncResult.Update(offset); - uploadCallback?.Invoke(offset); - }); + InternalUploadFile(input, path, flags, asyncResult, uploadCallback); asyncResult.SetAsCompleted(exception: null, completedSynchronously: false); } @@ -2200,7 +2186,7 @@ private List InternalSynchronizeDirectories(string sourcePath, string #region Existing Files at The Destination - var destFiles = InternalListDirectory(destinationPath, listCallback: null); + var destFiles = InternalListDirectory(destinationPath, asyncResult: null, listCallback: null); var destDict = new Dictionary(); foreach (var destFile in destFiles) { @@ -2268,13 +2254,14 @@ private List InternalSynchronizeDirectories(string sourcePath, string /// Internals the list directory. /// /// The path. + /// An that references the asynchronous request. /// The list callback. /// /// A list of files in the specfied directory. /// /// is . /// Client not connected. - private List InternalListDirectory(string path, Action listCallback) + private List InternalListDirectory(string path, SftpListDirectoryAsyncResult asyncResult, Action listCallback) { if (path is null) { @@ -2314,6 +2301,8 @@ private List InternalListDirectory(string path, Action listCallb f.Value)); } + asyncResult?.Update(result.Count); + // Call callback to report number of files read if (listCallback is not null) { @@ -2380,6 +2369,8 @@ private void InternalDownloadFile(string path, Stream output, SftpDownloadAsyncR totalBytesRead += (ulong) data.Length; + asyncResult?.Update(totalBytesRead); + if (downloadCallback is not null) { // Copy offset to ensure it's not modified between now and execution of callback @@ -2452,6 +2443,8 @@ private void InternalUploadFile(Stream input, string path, Flags flags, SftpUplo _ = Interlocked.Decrement(ref expectedResponses); _ = responseReceivedWaitHandle.Set(); + asyncResult?.Update(writtenBytes); + // Call callback to report number of bytes written if (uploadCallback is not null) { From 8b2103128fdc499a77144c22b3200d634d53d406 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Sat, 2 Dec 2023 19:58:12 +0100 Subject: [PATCH 2/2] Revert test --- .../OldIntegrationTests/SftpClientTest.Upload.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs b/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs index b4b620e77..91c248bd3 100644 --- a/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs +++ b/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs @@ -230,15 +230,7 @@ public void Test_Sftp_Multiple_Async_Upload_And_Download_10Files_5MB_Each() sftp.Disconnect(); Assert.IsTrue(hashMatches, "Hash does not match"); - if (!uploadDownloadSizeOk) - { - // TODO https://github.com/sshnet/SSH.NET/issues/1253 - Assert.Inconclusive("Uploaded and downloaded bytes should match, but test is not stable"); - } - else - { - Assert.IsTrue(uploadDownloadSizeOk, "Uploaded and downloaded bytes does not match"); - } + Assert.IsTrue(uploadDownloadSizeOk, "Uploaded and downloaded bytes does not match"); } }