Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v6]
On Fri, 18 Aug 2023 13:30:54 GMT, Weibing Xiao 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: > > re-write close method 1) Refactor the code again, and simplify the code to handle the setting of connection time 2) Add more test cases 3) Bring back the pre-created keystore. I could not find a good example to create the key store at runtime. - PR Comment: https://git.openjdk.org/jdk/pull/15294#issuecomment-1684688234
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v6]
On Fri, 18 Aug 2023 13:30:54 GMT, Weibing Xiao 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:
>
> re-write close method
Changes requested by aefimov (Reviewer).
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 293:
> 291: } else { // NO SocketFactory
> 292: // create a connected socket without factory
> 293: createConnectionSocket(endpoint, connectTimeout);
A socket returned by `createConnectionSocket` here needs to be saved to the
`socket` variable - currently almost all of the tests that are not using a
custom socket factory - fails with:
Caused by: java.lang.NullPointerException: Cannot invoke
"java.net.Socket.getInputStream()" because "this.sock" is null
at java.naming/com.sun.jndi.ldap.Connection.(Connection.java:238)
-
PR Review: https://git.openjdk.org/jdk/pull/15294#pullrequestreview-1585010623
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1298730216
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v6]
> 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: re-write close method - Changes: - all: https://git.openjdk.org/jdk/pull/15294/files - new: https://git.openjdk.org/jdk/pull/15294/files/3df1b6cc..ac72c3d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15294&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15294&range=04-05 Stats: 18 lines in 1 file changed: 1 ins; 13 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15294.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15294/head:pull/15294 PR: https://git.openjdk.org/jdk/pull/15294
