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

Reuse HttpClient and fix the wrong TLS and Proxy settings during MinioClient creation #641

Merged
merged 8 commits into from
Jul 24, 2022

Conversation

ebozduman
Copy link
Collaborator

Socket/Memory leak issue was addressed before, but there was a case where we were not using HttpHandler when MinioClient is created with no args.
Also adds a functional test for socket/memory leak issue.

@ebozduman
Copy link
Collaborator Author

Running dotnet regitlint --fail-on-diff for the build issue

@harshavardhana
Copy link
Member

Running dotnet regitlint --fail-on-diff for the build issue

You need fix, your formatting is all wrong for a .Net project source. Also you can't remove TLS skip for verification. Or you would have to trust the certs in CI/CD

@ebozduman
Copy link
Collaborator Author

OK. I'll add TLS back.

About formatting:
I use VisualStudio as my editor and I've disabled format on save setting, but it still forces format changes. I'll check it in again. I'll also make sure I manually run dotnet regitlint before pushing the changes going forward.

Minio.Functional.Tests/Program.cs Outdated Show resolved Hide resolved
Minio/MinioClient.cs Outdated Show resolved Hide resolved
Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

Some reviews

@ebozduman
Copy link
Collaborator Author

Implemented review comments and removed the socket/memory test per team decision.

Marked the parameterized construction method, MinioClient(HttpClient httpClient), obsolete. If user has this method used in his/her code, then s/he will see a message similar to the following warning message:

.../Minio.Functional.Tests/Program.cs(69,27): warning CS0618: 'MinioClient.MinioClient(HttpClient)' is obsolete: 
'Use MinioClient() and Builder method .WithHttpClient(httpClient)' [.../Minio.Functional.Tests/Minio.Functional.Tests.csproj]

Minio/MinioClient.cs Outdated Show resolved Hide resolved
Minio/MinioClient.cs Outdated Show resolved Hide resolved
@harshavardhana
Copy link
Member

I am not sure what is the bug fix here? @ebozduman - can you please explain?

@ebozduman
Copy link
Collaborator Author

Sorry for the confusion.
Things changed in time and the title and what has changed diverged.

TLDR: This PR will make the last 3 test cases reuse the existing HttpClient instance and fix the wrong TLS and Proxy settings when creating an instance of MinioClient.

Some more explanations:
After the original socket leakage problem is addressed (too many HttpClient instances were created instead of reusing the existing one), I wanted to create a test for it and add some extra clean up for the 3 tests that were still creating their own HttpClient (and MinioClient) instances. Those 3 tests depended on DownloadObjectAsync and UploadObjectAsync methods.

In the meantime, I discussed with Bala and like the other SDKs, we've decided a functional test for socket leakage is not necessary.

I also got the reviews from you for the unnecessary HttpHandler creation which was turning off TLS and the review about the proxy setting that causes not honoring local system proxies.

So, this PR will make the last 3 test cases to reuse the existing HttpClient instance and fix the wrong TLS and Proxy settings.
I'll change the title to reflect the correct definition of these changes.

@ebozduman ebozduman changed the title Improves the fix for socket/memory leak issue and adds a test for it Reuse HttpClient and fix the wrong TLS and Proxy settings during MinioClient creation Jul 24, 2022
@harshavardhana harshavardhana merged commit 9a9d202 into minio:master Jul 24, 2022
@ebozduman ebozduman mentioned this pull request Aug 9, 2022
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

2 participants