Skip to content

Allow creation of synchronous MinioClient using already created MinioAsyncClient #1685

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.

Sign up for GitHub

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

Open
jgolda wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow creation of synchronous MinioClient using already created MinioAsyncClient #1685

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/src/main/java/io/minio/MinioClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@
public class MinioClient implements AutoCloseable {
private MinioAsyncClient asyncClient = null;

private MinioClient(MinioAsyncClient asyncClient) {
protected MinioClient(MinioAsyncClient asyncClient) {
Copy link
Member

@balamurugana balamurugana Dec 26, 2025

Choose a reason for hiding this comment

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

If you are creating a different MinioAsyncClient and inherit MinioClient, why not have your own class inheriting MinioAsyncClient?

Copy link
Contributor Author

@jgolda jgolda Dec 26, 2025

Choose a reason for hiding this comment

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

I intend to use a class inheriting MinioAsyncClient - say CustomMinioAsyncClient. There I override the supplyAsync method, providing my own executor. It works fine.

The problem is I cannot create an instance of MinioClient nor it's subclass, which would be configured with a CustomMinioAsyncClient. The only constructor in MinioClient accepting MinioAsyncClient is private. This blocks me from crating a synchronous client (relatively easily), which will use a custom Executor

Copy link
Member

@balamurugana balamurugana Dec 26, 2025

Choose a reason for hiding this comment

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

What is the rationale behind using custom async and sync? why not use it directly?

Copy link
Contributor Author

@jgolda jgolda Dec 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In practise I only need the sync client. I override the async client only to provide a custom Executor, which executes everything on the calling thread (Runnable::run).

With the private constructor in MinioClient I can't reuse the already existing error handling logic in MinioClient and have to copy-paste this boilerplate to my project

Copy link
Member

@balamurugana balamurugana Dec 26, 2025

Choose a reason for hiding this comment

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

MinioClient just does

    try {
      return asyncClient.API(args).join();
    } catch (CompletionException e) {
      asyncClient.throwMinioException(e);
      return null;
    }

What do you mean by error handling logic in MinioClient? I do not think this PR adds value.

Copy link
Contributor Author

@jgolda jgolda Dec 26, 2025

Choose a reason for hiding this comment

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

I mean exactly what you posted. It's not much, but it has to be done for every method used from async client, adding significant amount of boilerplate to projects. It's especially frustrating that this code already exists and has to be copy pasted to the project and maintained for no good reason.

It would be really cool if it could be avoided by using existing code from the MinioClient.

Changing the constructor visibility is just my first idea - I'll b fine with a static factory method (eg. MinioClient.withAsyncClient(MinioAsyncClient) or some builder option if you prefer it

Copy link
Member

@balamurugana balamurugana Dec 26, 2025

Choose a reason for hiding this comment

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

I don't think you try/catch for every single call of async client. As the master is a breaking change, you anyway need to fix your code irrespective of this PR.

Copy link
Contributor Author

@jgolda jgolda Dec 26, 2025

Choose a reason for hiding this comment

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

Well I do try/catch for every method I use from async client, as I need to do more or less everything the sync client does. Now I'm copying parts of it to my code, which gives me 100-150 lines of boilerplate to support and merge with future min.io versions.

As far as I recall master didn't change much in terms of error handling, so aligning code to changes in master wouldn't improve much I guess. It won't remove the boilerplate, it might make it different.

You're clearly negative about this change. Does it break anything or would you like it done differently? I'm open to some different approach which will allow me to avoid the boilerplate, but I would need some directions on what should be improved

this.asyncClient = asyncClient;
}

Expand Down