Ver código fonte

Rebased AsyncResult fix (#1184)

* 🐛 AsyncResult contains invalid value

- 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

* Revert test

---------

Co-authored-by: Miroslav Pokorný <m.pokorny@quadient.com>
Rob Hague 1 ano atrás
pai
commit
6d9d03205b

+ 13 - 20
src/Renci.SshNet/SftpClient.cs

@@ -583,7 +583,7 @@ namespace Renci.SshNet
         {
             CheckDisposed();
 
-            return InternalListDirectory(path, listCallback);
+            return InternalListDirectory(path, asyncResult: null, listCallback);
         }
 
         /// <summary>
@@ -666,12 +666,7 @@ namespace Renci.SshNet
             {
                 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 @@ namespace Renci.SshNet
             {
                 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 @@ namespace Renci.SshNet
             {
                 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 @@ namespace Renci.SshNet
 
                 #region Existing Files at The Destination
 
-                var destFiles = InternalListDirectory(destinationPath, listCallback: null);
+                var destFiles = InternalListDirectory(destinationPath, asyncResult: null, listCallback: null);
                 var destDict = new Dictionary<string, ISftpFile>();
                 foreach (var destFile in destFiles)
                 {
@@ -2268,13 +2254,14 @@ namespace Renci.SshNet
         /// Internals the list directory.
         /// </summary>
         /// <param name="path">The path.</param>
+        /// <param name="asyncResult">An <see cref="IAsyncResult"/> that references the asynchronous request.</param>
         /// <param name="listCallback">The list callback.</param>
         /// <returns>
         /// A list of files in the specfied directory.
         /// </returns>
         /// <exception cref="ArgumentNullException"><paramref name="path" /> is <see langword="null"/>.</exception>
         /// <exception cref="SshConnectionException">Client not connected.</exception>
-        private List<ISftpFile> InternalListDirectory(string path, Action<int> listCallback)
+        private List<ISftpFile> InternalListDirectory(string path, SftpListDirectoryAsyncResult asyncResult, Action<int> listCallback)
         {
             if (path is null)
             {
@@ -2314,6 +2301,8 @@ namespace Renci.SshNet
                                             f.Value));
                 }
 
+                asyncResult?.Update(result.Count);
+
                 // Call callback to report number of files read
                 if (listCallback is not null)
                 {
@@ -2380,6 +2369,8 @@ namespace Renci.SshNet
 
                     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 @@ namespace Renci.SshNet
                                 _ = Interlocked.Decrement(ref expectedResponses);
                                 _ = responseReceivedWaitHandle.Set();
 
+                                asyncResult?.Update(writtenBytes);
+
                                 // Call callback to report number of bytes written
                                 if (uploadCallback is not null)
                                 {

+ 1 - 9
test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs

@@ -230,15 +230,7 @@ namespace Renci.SshNet.IntegrationTests.OldIntegrationTests
                 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");
             }
         }