Skip to content

Commit 6d9d032

Browse files
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>
1 parent 9b0262c commit 6d9d032

File tree

2 files changed

+14
-29
lines changed

2 files changed

+14
-29
lines changed

src/Renci.SshNet/SftpClient.cs

Lines changed: 13 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -583,7 +583,7 @@ public IEnumerable<ISftpFile> ListDirectory(string path, Action<int> listCallbac
583583
{
584584
CheckDisposed();
585585

586-
return InternalListDirectory(path, listCallback);
586+
return InternalListDirectory(path, asyncResult: null, listCallback);
587587
}
588588

589589
/// <summary>
@@ -666,12 +666,7 @@ public IAsyncResult BeginListDirectory(string path, AsyncCallback asyncCallback,
666666
{
667667
try
668668
{
669-
var result = InternalListDirectory(path, count =>
670-
{
671-
asyncResult.Update(count);
672-
673-
listCallback?.Invoke(count);
674-
});
669+
var result = InternalListDirectory(path, asyncResult, listCallback);
675670

676671
asyncResult.SetAsCompleted(result, completedSynchronously: false);
677672
}
@@ -898,12 +893,7 @@ public IAsyncResult BeginDownloadFile(string path, Stream output, AsyncCallback
898893
{
899894
try
900895
{
901-
InternalDownloadFile(path, output, asyncResult, offset =>
902-
{
903-
asyncResult.Update(offset);
904-
905-
downloadCallback?.Invoke(offset);
906-
});
896+
InternalDownloadFile(path, output, asyncResult, downloadCallback);
907897

908898
asyncResult.SetAsCompleted(exception: null, completedSynchronously: false);
909899
}
@@ -1131,11 +1121,7 @@ public IAsyncResult BeginUploadFile(Stream input, string path, bool canOverride,
11311121
{
11321122
try
11331123
{
1134-
InternalUploadFile(input, path, flags, asyncResult, offset =>
1135-
{
1136-
asyncResult.Update(offset);
1137-
uploadCallback?.Invoke(offset);
1138-
});
1124+
InternalUploadFile(input, path, flags, asyncResult, uploadCallback);
11391125

11401126
asyncResult.SetAsCompleted(exception: null, completedSynchronously: false);
11411127
}
@@ -2200,7 +2186,7 @@ private List<FileInfo> InternalSynchronizeDirectories(string sourcePath, string
22002186

22012187
#region Existing Files at The Destination
22022188

2203-
var destFiles = InternalListDirectory(destinationPath, listCallback: null);
2189+
var destFiles = InternalListDirectory(destinationPath, asyncResult: null, listCallback: null);
22042190
var destDict = new Dictionary<string, ISftpFile>();
22052191
foreach (var destFile in destFiles)
22062192
{
@@ -2268,13 +2254,14 @@ private List<FileInfo> InternalSynchronizeDirectories(string sourcePath, string
22682254
/// Internals the list directory.
22692255
/// </summary>
22702256
/// <param name="path">The path.</param>
2257+
/// <param name="asyncResult">An <see cref="IAsyncResult"/> that references the asynchronous request.</param>
22712258
/// <param name="listCallback">The list callback.</param>
22722259
/// <returns>
22732260
/// A list of files in the specfied directory.
22742261
/// </returns>
22752262
/// <exception cref="ArgumentNullException"><paramref name="path" /> is <see langword="null"/>.</exception>
22762263
/// <exception cref="SshConnectionException">Client not connected.</exception>
2277-
private List<ISftpFile> InternalListDirectory(string path, Action<int> listCallback)
2264+
private List<ISftpFile> InternalListDirectory(string path, SftpListDirectoryAsyncResult asyncResult, Action<int> listCallback)
22782265
{
22792266
if (path is null)
22802267
{
@@ -2314,6 +2301,8 @@ private List<ISftpFile> InternalListDirectory(string path, Action<int> listCallb
23142301
f.Value));
23152302
}
23162303

2304+
asyncResult?.Update(result.Count);
2305+
23172306
// Call callback to report number of files read
23182307
if (listCallback is not null)
23192308
{
@@ -2380,6 +2369,8 @@ private void InternalDownloadFile(string path, Stream output, SftpDownloadAsyncR
23802369

23812370
totalBytesRead += (ulong) data.Length;
23822371

2372+
asyncResult?.Update(totalBytesRead);
2373+
23832374
if (downloadCallback is not null)
23842375
{
23852376
// 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
24522443
_ = Interlocked.Decrement(ref expectedResponses);
24532444
_ = responseReceivedWaitHandle.Set();
24542445

2446+
asyncResult?.Update(writtenBytes);
2447+
24552448
// Call callback to report number of bytes written
24562449
if (uploadCallback is not null)
24572450
{

test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SftpClientTest.Upload.cs

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -230,15 +230,7 @@ public void Test_Sftp_Multiple_Async_Upload_And_Download_10Files_5MB_Each()
230230
sftp.Disconnect();
231231

232232
Assert.IsTrue(hashMatches, "Hash does not match");
233-
if (!uploadDownloadSizeOk)
234-
{
235-
// TODO https://github.com/sshnet/SSH.NET/issues/1253
236-
Assert.Inconclusive("Uploaded and downloaded bytes should match, but test is not stable");
237-
}
238-
else
239-
{
240-
Assert.IsTrue(uploadDownloadSizeOk, "Uploaded and downloaded bytes does not match");
241-
}
233+
Assert.IsTrue(uploadDownloadSizeOk, "Uploaded and downloaded bytes does not match");
242234
}
243235
}
244236

0 commit comments

Comments
 (0)