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.

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

Jump to bottom

VFS-824 HttpFileSystem free Unused Resources lead to HttpClient Conn… #300

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

VFS-824 HttpFileSystem free Unused Resources lead to HttpClient Conn… #300

wants to merge 15 commits into from

Conversation

mind-echo
Copy link

…ection Pool Shutdown

xieyuliang added 2 commits September 19, 2022 17:55
VFS-824 HttpFileSystem free Unused Resources lead to HttpClient Conne…
1a6685f 
…ction Pool Shutdown
VFS-824 Fix HttpFileSystem free Unused Resources lead to HttpClient C…
7d263ee 
…onnection Pool Shutdown

Option One：While Unused Resources, close HttpFileSystem
@garydgregory
Copy link
Member

https://issues.apache.org/jira/browse/VFS-824

VFS-824 Fix HttpFileSystem free Unused Resources lead to HttpClient C…
7c09877 
…onnection Pool Shutdown

Option Two：Share ConnectionManager.
@garydgregory
Copy link
Member

garydgregory commented Sep 19, 2022 •
edited
Loading

How about the http5 file provider?

@garydgregory
Copy link
Member

Hello @destroydestiny
Thank for your PR.
Please see my scattered comments.

@mind-echo
Copy link
Author

How about the http5 file provider?

Http, http4, http5 provider has the same problem

garydgregory
garydgregory requested changes Sep 19, 2022
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Please see my comments.

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileProvider.java Show resolved Hide resolved
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileProvider.java Outdated Show resolved Hide resolved
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileProvider.java Outdated Show resolved Hide resolved
@garydgregory
Copy link
Member

You must have not run a local build with the default Maven goal (mvn):

[ERROR] src/main/java/org/apache/commons/vfs2/provider/AbstractFileProvider.java:[19,17] (imports) AvoidStarImport: Using the '.*' form of import should be avoided - java.util.*.

garydgregory
garydgregory requested changes Sep 21, 2022
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hello @destroydestiny
Thank you for your updates. Please see my comments.

xieyuliang added 3 commits September 21, 2022 22:57
@mind-echo
Copy link
Author

Hello @destroydestiny Thank you for your updates. Please see my comments.

Hi @garydgregory
Thank you so much for the pointers! I need to ask you how to write code rigorously.
Cheers,

@garydgregory
Copy link
Member

It's possible one of these changes is breaking the build randomly.

@garydgregory
Copy link
Member

I need to look at this more closely over the weekend: I don't know why the HTTP providers have to be unique and different compared to all the others. Is the use of concepts in this PR backward? In the PR, the "free" code now also "closes" resources and that feels backward to me. I expect the "close" code to also "free" resources are part of closing, not the other way around.
Any thoughts?

@mind-echo
Copy link
Author

I need to look at this more closely over the weekend: I don't know why the HTTP providers have to be unique and different compared to all the others. Is the use of concepts in this PR backward? In the PR, the "free" code now also "closes" resources and that feels backward to me. I expect the "close" code to also "free" resources are part of closing, not the other way around. Any thoughts?

I think this problem needs to be considered from the beginning. The root cause is that "free" code closes httpClient.
Refer to org.apache.commons.vfs2.provider.http4.Http4FileSystem#doCloseCommunicationLink

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garydgregory garydgregory garydgregory requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mind-echo @garydgregory