Hello Sean, Daniel, Thank you for review I’ve added “com.sun.jndi.ldap.tls.cbtype” property into the module-info file and updated synchronisation using CompletableFuture. Also, I have added new test cases : successful and unsuccessful TLS handshake, synchronous and asynchronous TLS handshake.
New webrev at : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v13 Connection property is removed from CSR : https://bugs.openjdk.java.net/browse/JDK-8247311 Regards Alexey > On 8 Jul 2020, at 17:41, Daniel Fuchs <daniel.fu...@oracle.com> wrote: > > Thanks Sean, Alexey, > > On 08/07/2020 13:25, Sean Mullan wrote: >>> Updated webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v12/ >> You will also need to update the CSR to remove the connection timeout >> property. >> Also, you should document the "com.sun.jndi.ldap.tls.cbtype" property in the >> java.naming module-info file as was done for other properties in Daniel's >> recent RFR, once he has pushed it [1]. > > I have pushed the fix: > https://hg.openjdk.java.net/jdk/jdk/rev/ed375ae6c779 > > Alexey, you should now be able to document your new > "com.sun.jndi.ldap.tls.cbtype" > property in that @implNote as well, and update your CSR > consequently. > >> * src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java >> It looks like there is a possibility of deadlock if >> getTlsServerCertificate() is called before handshakeCompleted(). In that >> case the lock could be obtained first and the thread would end up holding >> the lock and waiting forever. > > I also have a concern with the use of wait/notify in that code. > I would suggest prototyping using a CompletableFuture instead > (or can new certificates be exchanged later on the same > connection?) > > CompletableFuture would allow you to relay and propagate any > exception raised in the callback handler as well, and would > avoid running into deadlocks. > > best regards, > > -- daniel
