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

Close HTTP client owned by MinioClient #1546

Conversation

findepi
Copy link
Contributor

@findepi findepi commented Apr 11, 2024

MinioClient builder and MinioAsyncClient builder allocate a new OkHttpClient instance. OkHttpClient manages internal resources such as dispatcher thread pool for executing HTTP requests and should be closed to dispose of these resources.

@@ -145,7 +146,8 @@ protected S3Base(
boolean useVirtualStyle,
String region,
Provider provider,
OkHttpClient httpClient) {
OkHttpClient httpClient,
boolean closeHttpClient) {
Copy link
Member

Choose a reason for hiding this comment

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

This breaks backward compatibility.

You would need to create another constructor and have this closeHttpClient argument and call the newly added constructor in this constructor.

Finally deprecate this constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, fixed!

@@ -193,6 +197,7 @@ protected S3Base(S3Base client) {
this.region = client.region;
this.provider = client.provider;
this.httpClient = client.httpClient;
this.closeHttpClient = false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.closeHttpClient = false;
this.closeHttpClient = client.closeHttpClient;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That depends on the purpose of this "copy" constructor, i.e. what semantics we want,
If the caller may use both instances of S3Base, only one should close the HTTP client (and then the other is not usable anymore).
If the caller is supposed to use only the new instance of S3Base, yes, we should copy the flag.

For now, I will copy the flag.

@findepi
Copy link
Contributor Author

findepi commented Apr 12, 2024

@balamurugana thank you for your review!

@findepi findepi force-pushed the findepi/close-http-client-owned-by-minioclient-b94a3b branch 2 times, most recently from 88bf15c to 0ed5c57 Compare April 12, 2024 07:08
MinioClient builder and MinioAsyncClient builder allocate a new
OkHttpClient instance. OkHttpClient manages internal resources such as
dispatcher thread pool for executing HTTP requests and should be closed
to dispose of these resources.
@findepi findepi force-pushed the findepi/close-http-client-owned-by-minioclient-b94a3b branch from 0ed5c57 to cc4d1b9 Compare April 12, 2024 07:10
@balamurugana
Copy link
Member

Please fix the CI failure

@findepi
Copy link
Contributor Author

findepi commented Apr 12, 2024

I see this failure on the CI

Error: Exception in thread "main" io.minio.errors.ServerException: server failed with HTTP status code 500
	at io.minio.S3Base$1.onResponse(S3Base.java:753)
	at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.kt:519)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:750)

I am able to reproduce the failure locally with (following the CI script

- name: Set environment variables
run: |
echo "DEV_VERSION=$(./gradlew properties | awk '/^version:/ { print $2 }')" >> $GITHUB_ENV
echo "RELEASE_VERSION=$(./gradlew properties -Prelease | awk '/^version:/ { print $2 }')" >> $GITHUB_ENV
- name: Gradle build on Ununtu
if: matrix.os == 'ubuntu-latest'
run: |
./gradlew build
./gradlew runFunctionalTest
javac -cp api/build/libs/minio-${DEV_VERSION}-all.jar functional/TestUserAgent.java
java -Dversion=${DEV_VERSION} -cp api/build/libs/minio-${DEV_VERSION}-all.jar:functional TestUserAgent
)

./gradlew clean build && 
DEV_VERSION="$(./gradlew properties | awk '/^version:/ { print $2 }')" && 
javac -cp api/build/libs/minio-${DEV_VERSION}-all.jar functional/TestUserAgent.java &&
java -Dversion=${DEV_VERSION} -cp api/build/libs/minio-${DEV_VERSION}-all.jar:functional TestUserAgent

however, I think I am doing something wrong still, as the above fails also when I run it on master.
@balamurugana please help me what should be the right command to reproduce the CI failure

@balamurugana
Copy link
Member

@findepi TestUserAgent.java fails due to example.org server failure. Could you apply below diff to this PR?

diff --git a/functional/TestUserAgent.java b/functional/TestUserAgent.java
index fca96cfb..51180f99 100644
--- a/functional/TestUserAgent.java
+++ b/functional/TestUserAgent.java
@@ -23,7 +23,7 @@ import java.util.Scanner;
 
 public class TestUserAgent {
   public static void main(String[] args) throws Exception {
-    MinioClient client = MinioClient.builder().endpoint("http://example.org").build();
+    MinioClient client = MinioClient.builder().endpoint("http://httpbin.org").build();
     ByteArrayOutputStream baos = new ByteArrayOutputStream();
     client.traceOn(baos);
     client.bucketExists(BucketExistsArgs.builder().bucket("any-bucket-name-works").build());

@findepi
Copy link
Contributor Author

findepi commented Apr 13, 2024

@balamurugana done!

@harshavardhana harshavardhana merged commit 4c33cfd into minio:master Apr 14, 2024
8 checks passed
@findepi findepi deleted the findepi/close-http-client-owned-by-minioclient-b94a3b branch April 14, 2024 06:29
@findepi
Copy link
Contributor Author

findepi commented Apr 14, 2024

@balamurugana thanks for review!
@harshavardhana thanks for the merge!

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.

None yet

3 participants