Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use IAsyncEnumerable for ListObjectsAsync instead of observables. #1049

Merged
merged 8 commits into from
May 23, 2024

Conversation

ramondeklein
Copy link
Contributor

@ramondeklein ramondeklein commented Apr 9, 2024

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 ramondeklein self-assigned this Apr 9, 2024
var subscription = observable.Subscribe(
item => Console.WriteLine($"Object: {item.Key}"),
ex => Console.WriteLine($"OnError: {ex}"),
() => Console.WriteLine($"Listed all objects in bucket {bucketName}\n"));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

@ebozduman ebozduman Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@ramondeklein ramondeklein marked this pull request as draft April 9, 2024 18:54
@ramondeklein ramondeklein removed the request for review from ebozduman April 9, 2024 18:55
@martijn00
Copy link
Contributor

@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
Copy link
Contributor Author

@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...

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Minio/ApiEndpoints/BucketOperations.cs Outdated Show resolved Hide resolved
ebozduman
ebozduman previously approved these changes Apr 15, 2024
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Marked Approve.

@ramondeklein
Copy link
Contributor Author

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

@martijn00
Copy link
Contributor

@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
Copy link
Contributor Author

@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 ramondeklein marked this pull request as ready for review May 22, 2024 09:45
@ramondeklein
Copy link
Contributor Author

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

@harshavardhana harshavardhana merged commit cb74b1a into minio:master May 23, 2024
7 checks passed
@ebozduman
Copy link
Collaborator

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

@delasource
Copy link

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
Copy link
Contributor Author

@ebozduman Can we release the latest master to NuGet?

@ebozduman
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants