RFR  8217606: LdapContext#reconnect always opens a new connection
Roger.Riggs at oracle.com
Wed Aug 7 15:52:49 UTC 2019
Nicely structured server framework, some details to cleanup.
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
Since it is called in a Executor, otherwise unhandled exceptions will
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()
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
184: hexToBin(ch) -> Character.digit(ch, 16);
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
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?
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:
>> Please review the following change:
>> The original issue seems to be an unintended consequence of JDK-8059009 .
>> 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 
>> - 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.
>>  https://bugs.openjdk.java.net/browse/JDK-8059009
>>  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>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>
More information about the core-libs-dev