On Tue, 22 Aug 2023 18:21:17 GMT, Weibing Xiao <d...@openjdk.org> wrote:
>> Please refer to JDK-8314063.
>>
>> The failure scenario is due to the setting of connection timeout. It is
>> either too small or not an optimal value for the system. When the client
>> tries to connect to the server with LDAPs protocol. It requires the
>> handshake after the socket is created and connected, but it fails due to
>> connection timeout and leaves the socket open. It is not closed properly due
>> to the exception handling in the JDK code.
>>
>> The change is adding a try/catch block and closing the socket in the catch
>> block, and the format of the code got changed consequently.
>
> Weibing Xiao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> format the code
May I suggest a slightly different approach for refactoring of the Connection
class code here:
Instead of having two methods for creating a `Socket` with OR without socket
factory the method to select a socket factory can be introduced:
If a socket factory class name is not set - the factory returned by
`SocketFactory.getDefault()` will be used (which is identical to `s = new
Socket()` for calls with timeout set; and `s = new Socket(host, port)` for
calls with no timeout set):
``` private SocketFactory getSocketFactory(String socketFactoryName) throws
Exception {
if (socketFactoryName == null) {
if (debug) {
System.err.println("Connection: using default SocketFactory");
}
return SocketFactory.getDefault();
} else {
if (debug) {
System.err.println("Connection: loading supplied SocketFactory: " +
socketFactoryName);
}
@SuppressWarnings("unchecked")
Class<? extends SocketFactory> socketFactoryClass =
(Class<? extends SocketFactory>)
Obj.helper.loadClass(socketFactoryName);
Method getDefault =
socketFactoryClass.getMethod("getDefault");
SocketFactory factory = (SocketFactory) getDefault.invoke(null, new
Object[]{});
return factory;
}
}
It then can be used to create a connection socket:
private Socket createConnectionSocket(String host, int port, SocketFactory
factory, int connectTimeout) throws Exception {
Socket socket = null;
if (connectTimeout > 0) {
// create unconnected socket and then connect it if timeout
// is supplied
InetSocketAddress endpoint =
createInetSocketAddress(host, port);
// unconnected socket
socket = factory.createSocket();
// connect socket with a timeout
socket.connect(endpoint, connectTimeout);
if (debug) {
System.err.println("Connection: creating socket with " +
"a connect timeout");
}
}
if (socket == null) {
// create connected socket
socket = factory.createSocket(host, port);
if (debug) {
System.err.println("Connection: creating connected socket with"
+
" no connect timeout");
}
}
return socket;
}
And then `createSocket` can be changed to:
private Socket createSocket(String host, int port, String socketFactory,
int connectTimeout) throws Exception {
SocketFactory factory = getSocketFactory(socketFactory);
assert factory != null;
Socket socket = createConnectionSocket(host, port, factory,
connectTimeout);
// the handshake for SSL connection with server and reset timeout for
the socket
try {
initialSSLHandshake(socket, connectTimeout);
} catch (Exception e) {
// 8314063 the socket is not closed after the failure of handshake
// close the socket while the error happened
closeOpenedSocket(socket);
throw e;
}
return socket;
}
Also, the new try catch block can be put around initialSSLHandshake method only
- it is the only case when socket can be set to a non-null value.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/15294#issuecomment-1690278791
