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

NET-685: Update SocketClient default connect timeout from ∞ to 60 seconds #51

Merged
merged 1 commit into from Nov 29, 2020

Conversation

simo385
Copy link

@simo385 simo385 commented Jul 21, 2020

I changed the socket DEFAULT_CONNECT_TIMEOUT value, because, if the server doesn't reply and I don't set the connection timeout, the method FTPClient.connect() hangs infinitely, blocking the whole JVM.

This happens in the multi-threading environment.

Even though there exist the method setConnectTimeout() to set the timeout for Socket, I think a default value should be set-up in order to avoid production breakdown.

Thanks to everybody.
Simone

@garydgregory
Copy link
Member

@garydgregory
Copy link
Member

@simo385
I am not opposed to the idea but note that SocketClient is an abstract class with many subclasses. I am worried that there are some unintended functional side-effects that this change will cause. Any thoughts from others? I suppose your workaround is to call org.apache.commons.net.SocketClient.setConnectTimeout(int)?

@simo385
Copy link
Author

simo385 commented Nov 23, 2020

Dear @garydgregory,

yes, currently, the workaround is to call org.apache.commons.net.SocketClient.setConnectTimeout(int), but I've had a lot of problem in my production environment (multi-threading) because of infinity timeout. Also, I've spent a lot of time to find out the issue.

I understand your worry, we can discuss the default timeout value, but IMHO to set a default timeout = infinity is really dangerous, especially in multi-threading environment.

Many thanks for your support.
Simone

@garydgregory
Copy link
Member

Since no one has piped up against this and it seems like a good idea, I will merge.

@garydgregory garydgregory merged commit b9d0fcd into apache:master Nov 29, 2020
@garydgregory garydgregory changed the title NET-685: change default connect timeout NET-685: change SocketClient default connect timeout Nov 29, 2020
@garydgregory garydgregory changed the title NET-685: change SocketClient default connect timeout NET-685: Update SocketClient default connect timeout from ∞ to 60 seconds Nov 29, 2020
asfgit pushed a commit that referenced this pull request Nov 29, 2020
@simo385 simo385 deleted the NET-685-connect-timeout branch January 22, 2021 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants