Add connection timeout support to SocketClient and/or SocketFactory/DefaultSocketFactory

• Key: NET-141
• Type: Improvement
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Daniel Savarese
• Reporter: Christian Hufgard
• Affects Version/s: 1.4
• Fix Version/s: 2.0

Description

Hi,

If executing the following code
String hostname = "localhost";
FTPClient client = new FTPClient();
client.setDefaultTimeout(1000);
client.connect(hostname);

against a ftp server that ignores the connection attempt (e.g. is firewalled/malfunctoned), there will be no exception after 1000 ms. The exception will be thrown after a default timeout of three minutes. (Three minutes on a debian/ and a suse machines. Might be different on other platforms).

JavaDoc says:
public void setDefaultTimeout(int timeout)
Set the default timeout in milliseconds to use when opening a socket.

Digging through the code I found, that DefaultSocketFactory which is used be SocketClient does not care about any value set with this method. It creates a new Socket with Socket(hostname, port) and relies on the VMs behaviour.

To get this fixed I set a custom SocketFactory with client.setSocketFactory(socketFactory); that uses a timeout for socket connection.

This bug is also in 1.4.1, but this value is not listed...

Christian

Comments

Simple factory that uses jdk1.4's feature to create a Socket with a connection timeout.

[Daniel Savarese]

This is not a bug. You used the API exactly as intended. If you want a connect timeout, you need to create a socket factory (this has been explained multiple times on th mailing list). Commons Net 1.4.1 is JDK 1.3 compatible. Therefore, it cannot support connect timeouts. setDefaultTimeout does exactly what it is supposed to do. It sets the socket timeout (read/write timeout). For a release that supports JDK 1.4 or 1.5 as a baseline, connect timeouts will be added.

[Christian Hufgard]

Thanks for the quick response.

Ok, should have checked the archives before . But I think if this is a feature, it should be documented within the API. At least I haven't found it within FTPClient and SocketClient. DefaultSocketFactory just notices that it wrappes Socket.

Maybe SocketClient.setDefaultTimeout(int timeout) would be a good place to get this information noticed.

I still think, this behaviour is a little bit strange. We also use commons-httpclient and there is some code that uses >jdk 1.4 feature if available and implements a custom way for the connect timeout if they are not.
Think this would be usefull for ftpclient too.

[Daniel Savarese]

I'm reclassifying this issue as an improvement slated for the 2.0 release, which is allowed to use JDK 1.4/1.5 features. We've got to discuss on commons-dev a bit how best to support connection timeouts. Do we want to add a slew of connect methods to SocketClient that take timeout parameters (this would be nicer if Java supported default argument values)? Or do we want to add a setConnectionTimeout (easier change, but awkward)?

My bias is to add only one or two connect methods that correspond to the two Socket.connect methods that require a SocketAddress argument. This avoids having a ton of different version of connect. Another thing to consider (separate issue) is if we need to deprecate SocketFactory and move to javax.net.SocketFactory and javax.net.ServerSocketFactory. Unfortunately, the javax.net classes are not interfaces, so I don't think we can abandon SocketFactory. However, as part of this issue, a createSocket() method must be added that creates an unconnected Socket. We use that to create the socket and then the JDK 1.4 Socket.connect to establish the socket connection with a timeout.

At any rate, that's my proposal (two new SocktClient.connect methods and one new SocketFactory/DefaultSocketFactory.createSocket() method) . If Rory, Steve, and company don't object, I'll make the change to the 2.0 tree either this weekend or the following weekend.

[Rory Winston]

Added in 2.0.