Hi Alexey,
Thanks for addressing comments and answering questions. The JNDI changes
in latest webrev looks good to me.
CI is also happy with the latest changes.
Best,
Aleksei
On 10/07/2020 21:37, Alexey Bakhtin wrote:
Hello Aleksei,
Thank you for review.
Please see my comments below.
Updated webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v14/
Regards
Alexey
On 10 Jul 2020, at 19:40, Aleks Efimov <aleksej.efi...@oracle.com> wrote:
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 :)
HandshakeCompletedListener is called in case of successful handshake only.
In case of async handshake failed we close the connection and complete CF
exceptionally to release CF.get()
The ExecutionException could be unpacked instead of using it directly - and its
cause used instead.
Thank you. Fixed in Connection.java and LdapCBPropertiesTest.java
'getTlsServerCertificate' should return null if 'isTlsConnection()' is false -
rather than waiting forever.
We call isTlsConnection() in the LdapSasl before getTlsServerCertificate().
But your are right, we can double check it to prevent possible deadlock in the
future, if code changed.
Fixed in Connection.java
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.
Done
Connection.java:995 - you could use diamond operator here
Updated
Formatting: Connection.java:1027 'catch (‘
Updated
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.
Thank you for suggestion. Updated to use BaseLdapServer in the test
LdapCBPropertiesTest.java:122 - could use no param Hastable constructor
Fixed
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
