minio/minio-dotnet

`PutObject_Test9` sometimes fails during CI/CD build

ramondeklein opened this issue · 2 comments

It looks like the PutObject_Test9 sometimes fails with the following error:

Unhandled exception. Microsoft.VisualStudio.TestTools.UnitTesting.AssertFailedException: Assert.IsTrue failed. 
   at Microsoft.VisualStudio.TestTools.UnitTesting.Assert.ThrowAssertFailed(String assertionName, String message) in /_/src/TestFramework/TestFramework/Assertions/Assert.cs:line 60
   at Microsoft.VisualStudio.TestTools.UnitTesting.Assert.IsTrue(Boolean condition, String message, Object[] parameters) in /_/src/TestFramework/TestFramework/Assertions/Assert.IsTrue.cs:line 97
   at Microsoft.VisualStudio.TestTools.UnitTesting.Assert.IsTrue(Boolean condition) in /_/src/TestFramework/TestFramework/Assertions/Assert.IsTrue.cs:line 26
   at Minio.Functional.Tests.FunctionalTest.PutObject_Test9(IMinioClient minio) in /_/Minio.Functional.Tests/FunctionalTest.cs:line 3526
   at Minio.Functional.Tests.FunctionalTest.PutObject_Test9(IMinioClient minio) in /_/Minio.Functional.Tests/FunctionalTest.cs:line 3543
   at Minio.Helper.Utils.ForEachAsync[TSource](IEnumerable`1 source, Boolean runInParallel, Int32 maxNoOfParallelProcesses) in /_/Minio/Helper/Utils.cs:line 235
   at Minio.Functional.Tests.Program.Main(String[] args) in /_/Minio.Functional.Tests/Program.cs:line 254
   at Minio.Functional.Tests.Program.<Main>(String[] args)

I think I see the problem... Reporting is done using the System.Progress<T> class. As can be read here these events are send to the synchronization context of the Progress<T> object during construction. This is the actual call that is being invoked when you call Progress.Report(...):

/// <summary>Reports a progress change.</summary>
/// <param name="value">The value of the updated progress.</param>
void IProgress<T>.Report(T value) { OnReport(value); }

/// <summary>Reports a progress change.</summary>
/// <param name="value">The value of the updated progress.</param>
protected virtual void OnReport(T value)
{
    // If there's no handler, don't bother going through the sync context.
    // Inside the callback, we'll need to check again, in case
    // an event handler is removed between now and then.
    Action<T>? handler = _handler;
    EventHandler<T>? changedEvent = ProgressChanged;
    if (handler != null || changedEvent != null)
    {
        // Post the processing to the sync context.
        // (If T is a value type, it will get boxed here.)
        _synchronizationContext.Post(_invokeHandlers, value);
    }
}

As you can see, it will perform a Post(...) on the synchronization context and that means that it will be invoked asynchronously. A Post(...) is a kind of fire-and-forget mechanism. So if your code runs fast, then the progress update is still in the queue and waiting to be dispatched, but the upload is already finished.

The reason that MS decided to use asynchronous posting of events in System.Progress<T> is probably, because progress is often used to provide feedback to the user. Most UI frameworks (at least WinForms and WPF) require that all UI updates are done on the main-thread. Because progress updates are normally not needed to be done in real-time (it doesn't matter that a progress updates is a few milliseconds late), using the SynchronizationContext is very convenient in UI applications.

The reason that they choose to Post() instead of Send() is not to block the caller. If they would have used Send(), then the call will be called synchronously on the progress thread and that would block processing and slow it down.

In a unit-test, there is no need for thread-synchronisation and using a callback that is invoked on an arbitrary thread is just fine.