Hi Alexey,
Thank you for removing the dependency on the timeout property and adding
tests for TLS handshake cases.
Please, find the comments about the latest webrev below:
Not quite sure why the CF is completed at two places. Probably that’s
OK, but it would be good to know the reason :)
The ExecutionException could be unpacked instead of using it directly -
and its cause used instead.
'getTlsServerCertificate' should return null if 'isTlsConnection()' is
false - rather than waiting forever.
java.naming/share/classes/module-info.java: could we try to improve the
formatting of the possible values for the new system property - maybe
format them as a list.
Connection.java:995 - you could use diamond operator here
Formatting: Connection.java:1027 'catch ('
Could we use the test/jdk/com/sun/jndi/ldap/lib/BaseLdapServer.java from
the test library to implement dummy LDAPS server, I believe it could
help to increase the test verbosity and reduce its code size.
LdapCBPropertiesTest.java:122 - could use no param Hastable constructor
I've also run your latest patch through our CI system and it showed no
failures with your latest changes.
-Aleksei
On 09/07/2020 20:34, Alexey Bakhtin wrote:
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
