RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v3]

[Mark Sheppard]

On Wed, 16 Aug 2023 23:11:11 GMT, Weibing Xiao <duke at 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:
> 
>   updated the code according to the review

you now have two methods closing a socket. Can these be refactored and a single closeSocket method used in both cases ?

method overloading could have been used for the new create socket method

you might consider some restructuring of the call flow in each:

   private Socket createSocketWithoutFactory (String host, int port, int connectTimeout) throws Exception {
        Socket socket = null;
        InetSocketAddress endpoint = createInetSocketAddress(host, port);
        socket = new Socket();

        if (connectTimeout > 0) {
            if (debug) {
                System.err.println("Connection: creating socket with " +
                        "a timeout");
            }
            socket.connect(endpoint, connectTimeout);
        } else {
            if (debug) {
                System.err.println("Connection: creating socket");
            }
            // connected socket
            socket.connect(endpoint);
        }
        return socket;
    }


    // create the socket with the provided factory
    private Socket createSocketWithFactory(String host, int port, String socketFactory,
                                           int connectTimeout) throws Exception {
        Socket socket = null;
        @SuppressWarnings("unchecked")
        Class<? extends SocketFactory> socketFactoryClass = (Class<? extends SocketFactory>)
                Obj.helper.loadClass(socketFactory);
        Method getDefault =
                socketFactoryClass.getMethod("getDefault", new Class<?>[]{});
        SocketFactory factory = (SocketFactory) getDefault.invoke(null, new Object[]{});
        InetSocketAddress endpoint =
                  createInetSocketAddress(host, port);
        // create unconnected socket
        socket = factory.createSocket();

        // create the socket
        if (connectTimeout > 0) {
            if (debug) {
                System.err.println("Connection: creating socket with " +
                        "a timeout using supplied socket factory");
            }
            // connected socket
            socket.connect(endpoint, connectTimeout);
        } else {
            if (debug) {
                System.err.println("Connection: creating socket using " +
                        "supplied socket factory");
            }
            // connected socket
            socket.connect(endpoint);
        }
        return socket;
    }

-------------

PR Comment: https://git.openjdk.org/jdk/pull/15294#issuecomment-1681979781