RFR [14] 8217606: LdapContext#reconnect always opens a new connection

[Roger Riggs]

Hi,

Nicely structured server framework, some details to cleanup.

BaseLdapServer:

100: The new exception should have a message "Unexpected exception" or 
"server should no be running"...
158: Is printing the stack trace diagnostic or an error?, the exception 
is not rethrown and no message clarifying
the context of the trace is printed.
Stack traces should go to the log as well or instead of stderr.

103: plural  "accepting connection" -> "accepting connections"

109: "template method" doesn't describe the method well, the method is 
private and not overridable.
    update the javadoc.

154: Handle connection should handle  Throwable and log and error with a 
printStackTrace.
Since it is called in a Executor, otherwise unhandled exceptions will 
dissappear.

178:  logger() should be final. The subclass has no reason to override.  
And it can be "public final".

238: isRunning should be synchronized(lock) to make sure each Thread 
sees the same value threads that update it in start() and close()


LdapMessage:

90:  plural?  byte[] messages implies multiple messages, not just a 
buffer for a single message.

136: Why make a copy?  Is it expected to be modified?  Be clear in the 
javadoc about the copy.

230: can the same trick using BigInteger (line 124) be used to extract 
the length?

184:  hexToBin(ch) -> Character.digit(ch, 16);



Reconnect:

54: there is no need for a stylized new Reconnect().run().
Just call a static method and keep the test simple.

105:  Tests that sleep are prone to intermittent failures on slow or 
delayed systems.

It would be more reliable to countdown *after* the Connection was handled.
As is, it might report success even if handleRequest failed for some reason.

Since the test only needs to know that the connection is made, a 
CountDownLatch could be used.
Await() with a large timeout would complete as soon as the connection 
was made and still
catch the case where it never happened.  If there as some suspecion of 
too many connections
that could be checked after the beforeConnectionHandled called countDown.


Since new infra structure is being introduced, it should be considered 
to leverage TestNG
testing facilities and Asserts?

Regards, Roger


On 8/7/19 9:30 AM, Lance Andersen wrote:
> Hi Pavel,
>
> The change looks good.  Nice job on the tests :-)
>
>
>> On Aug 7, 2019, at 8:47 AM, Pavel Rappo <pavel.rappo at oracle.com> wrote:
>>
>> Hello,
>>
>> Please review the following change:
>>
>>    http://cr.openjdk.java.net/~prappo/8217606/webrev.00/
>>
>> The original issue seems to be an unintended consequence of JDK-8059009 [1].
>> The proposed change is based on the private patch by Chris Yin. I skimmed
>> through the history of the `com.sun.jndi.ldap.Connection.useable` flag which
>> dates back to JDK-4532408 [2]
>>
>>     - Maintains a flag, 'useable', to indicate whether underlying network
>>     connection is useable. A connection starts out useable and becomes
>>     unuseable when an IO error is encountered while reading from/writing
>>     to it.
>>
>> and I believe that this field is a good fit for what Chris has proposed.
>>
>> Not only does the proposed change fix the issue, it also introduces a reusable
>> test component for mocking/stubbing/faking an LDAP server. Chris and myself took
>> some time in our private correspondence to shape that component such that it
>> would allow to refactor some of the existing LDAP tests, should we decide to
>> do so.
>>
>> As far as mach5 is concerned, the change looks good.
>>
>> Thanks,
>> -Pavel
>>
>> -------------------------------------------------------------------------------
>> [1] https://bugs.openjdk.java.net/browse/JDK-8059009
>> [2] https://bugs.openjdk.java.net/browse/JDK-4532408
>>
>   <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>   <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>   <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>
>
>