Use IAsyncEnumerable for ListObjectsAsync instead of observables.

[ramondeklein]

Using IAsyncEnumerable is much easier to work with compared to IObservable, so I have rewritten the ListObjectsAsync method. This also fixes a bug that causes Minio.Examples.Cases.ListObjects.Run to return before it has actually completed. The code is also much simpler and more efficient.

Using IAsyncEnumerable has a significant advantage over IEnumerable, because it can be cancelled and doesn't need to finish until all objects have been loaded. It actually hides the paging-mechanism to the caller, but under the hood it's still being used.

To avoid breaking changes, I have used a new name for the new method and created an extension method (with the old name) that converts the IAsyncEnumerable back to IObservable (zero cost), but marked it as obsolete to encourage clients to use the updated method.

[ramondeklein]

This code had several bugs:

1. The method returns directly after subscribing, so the events are emitted when the function has already completed.
2. The subscription is never disposed (but if used using var subscription == ..., then the subscription would be cancelled when it got out-of-scope (directly after subscribing).

[ebozduman]

Very well.
I've been looking for a way to replace all the subscribe/observable blocks as they mostly have the issues you've explained above.

The last one I've tried and replaced an instance of a Subscribe/Observe code block is as follows:

            foreach (var item in minio.ListObjectsAsync(listObjectsArgs).TakeWhile((item, indx) => item is not null))
            {
                items.Add(item);
                :
                :

So, LGTM!
As you've wrote, this will eliminate the timing issues and other problems IObservable has.

[ramondeklein]

Changing an IObservable<T> to an IAsyncEnumerable<T> can be done with a helper from System.Linq.Async.ToAsyncEnumerable. You can then use await foreach, but not sure if we will lose .NET Framework compatibility.

[ebozduman]

Yes. It is important that we keep supporting old .NET Framework versions.
I'll start working on it and look into and fix if any problems.

[martijn00]

I would drop anything below .NET 6: https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core
It is worth supporting long term support like that, and .NET 4.8.x also has LTS, but is compatible with much newer APIs.

There is not many valid reasons anyone would continue to use .netstandard2.x.

[ramondeklein]

I think we should keep this SDK compatible with .NET Framework and move it into maintenance mode (so only bugfixing and security fixes). For new functionality, I think for we should rewrite the SDK and focus on .NET 6 (and later). Rewriting the SDK is less work compared to getting this SDK up to modern .NET standards. Modern .NET requires support for the extension libraries (DI, logging, configuration, ...).

My PoC supports .NET Core 6.0 and up. It allows for much more scalable software (more async functions). I don't think anyone started a .NET Framework project the past 3-5 years anymore, so there is no need to support .NET Framework for new developments.

I think I'll drop the unit-tests for Minio functionality from the PoC, because the integration tests are easier to maintain and also guarantee proper functionality. Only unit tests for basic things (like AWS V4 authentication) seem legit.

[ramondeklein]

@martijn00 Developing a new .NET SDK won't have the highest priority (if it's decided to even continue to work on it), so I think .NET 9 will be around when the new SDK released. But I currently did implement shims for missing .NET 6 functionality. It is recommended to use .NET 7, because it won't use shims.

[martijn00]

@ramondeklein this is great! I made several attempts at IAsyncEnumerable in the past already. It would be great to remove the dependency on Reactive since it is not maintained that much.

[ramondeklein]

@ramondeklein this is great! I made several attempts at IAsyncEnumerable in the past already. It would be great to remove the dependency on Reactive since it is not maintained that much.

I'm working on a proof-of-concept to rewrite the Minio SDK for .NET, but not sure if we are going through with it.

@martijn00 Enjoy your "kapsalon" and say ✋🏻 to Thomas. Greetings from Enschede...

[ebozduman]

LGTM.

@ramon,
Did you run Example1 and/or the other tests ?
If yes, could you add some simple info about them in the PR, like how you ran the tests and the expected results, etc.

[ebozduman]

LGTM
Marked Approve.

[ramondeklein]

@ebozduman I see that I need to fix some tests in the functional tests. Will do that later this week.

[martijn00]

@ramondeklein this is probably not the right place, but if you want to pick up something i haven't done yet in all the refactoring: https://github.com/minio/minio-dotnet/blob/master/Directory.Build.props#L21

Enable nullable and fix all the issues. This would help with quite some bugs.

[ramondeklein]

@ebozduman I also added the user metadata changes from #1043 to this PR. There is a compatibility extension, so code that is based on IObservable<> should still work.

[ramondeklein]

The updated version also doesn't need any delays anymore (I've removed them from the tests) and all tests are still working.

[ebozduman]

I just finished running my verification tests and my review.
I was going to approve the PR, but
@harshavardhana beats again.

[delasource]

when will this be released? The official quick start documentation links to a file changed in this PR, but that code (i.e. the newly named function ListObjectsEnumAsync) is not available in the latest release version.

Aka the documentation is in a wrong state until this releases.

[ramondeklein]

@ebozduman Can we release the latest master to NuGet?

[ebozduman]

Yes, we can as soon as the regression issue, PR#1106, is resolved.
I am looking into your review comments. I will implement/fix according to your comments and ask your review again.
Then, we can merge and create a new nuget package.