Close HTTP client owned by MinioClient #1546
Conversation
| Provider provider, | ||
| OkHttpClient httpClient) { | ||
| OkHttpClient httpClient, | ||
| boolean closeHttpClient) { |
There was a problem hiding this comment.
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.
| this.region = client.region; | ||
| this.provider = client.provider; | ||
| this.httpClient = client.httpClient; | ||
| this.closeHttpClient = false; |
There was a problem hiding this comment.
| this.closeHttpClient = false; | |
| this.closeHttpClient = client.closeHttpClient; |
There was a problem hiding this comment.
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.
|
@balamurugana thank you for your review! |
88bf15c to
0ed5c57
Compare
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.
0ed5c57 to
cc4d1b9
Compare
|
Please fix the CI failure |
|
I see this failure on the CI I am able to reproduce the failure locally with (following the CI script minio-java/.github/workflows/gradle.yml Lines 34 to 44 in fcbfa54 ./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 TestUserAgenthowever, I think I am doing something wrong still, as the above fails also when I run it on master. |
|
@findepi 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()); |
|
@balamurugana done! |
|
@balamurugana thanks for review! |
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.