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

[Roger Riggs]

Hi Pavel, Chris,

Looks good,

Thanks for the improvements and cleanup as well as fixing the bug.

Roger


On 8/13/19 2:21 PM, Pavel Rappo wrote:

On 13 Aug 2019, at 16:58, Roger Riggs  wrote:



The Semaphore implementation is the least complex.  Looks fine to me.

Updated:

 http://cr.openjdk.java.net/~prappo/8217606/webrev.01/

-Pavel

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

[Pavel Rappo]

> On 13 Aug 2019, at 16:58, Roger Riggs  wrote:
> 
> 
> 
> The Semaphore implementation is the least complex.  Looks fine to me.

Updated:

http://cr.openjdk.java.net/~prappo/8217606/webrev.01/

-Pavel

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

[Roger Riggs]

Hi Pavel,

On 8/12/19 1:41 PM, Pavel Rappo wrote:

Comments are inline.


On 8 Aug 2019, at 18:59, Roger Riggs  wrote:

...

That's fine, a Semaphore can be used to wait for the first connection and then 
check with a different timeout for unexpected connections.

I've sketched some possible implementations of the hybrid check we talked about. Even though the 
"Semaphore option" looks the most concise, it simultaneously looks the most 
"off-label" and counterintuitive (to my liking). However, I'm fine with it.


The Semaphore implementation is the least complex.  Looks fine to me.

Thanks for the prototypes.

Roger



-Pavel

// --

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Phaser;
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicInteger;

public class TwoPhase {

 public static void main(String[] args) throws InterruptedException {
 final Option opt = new OptionA();
//final Option opt = new OptionB();
//final Option opt = new OptionC();
 opt.acceptConnection();
 opt.accep

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


True, however, I'm not aware of any general solution of this problem.

General is not needed, and sleeps waste time time and resources.
In this particular test, if there is no possiblity of multiple connections then 
CountDownLatch is a good mechanism.

I would prefer to test for a particular number of connections.

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.


Hm... Let me think. What we are interested in here is whether the connection is 
attempted or not. We may do what you are suggesting just to be sure the server 
is not failing. That is, the client is served successfully.

The test description is not specific about that.
And it raises questions about the purpose of the environment setup.
Is it all needed.

I will update the documentation.

Is handling Unbind in the switch needed (as different from the default).

Chris might want to answer that.



If there as some suspecion of too many connections
that could be checked after the beforeConnectionHandled called countDown.


Wouldn't that make problems of its own? If I got you right, a time window for 
the LDAP client to create more *unwanted* connections could be really small. A 
hybrid approach might be even better. We wait for the first connection with a 
generous timeout and then give the client some extra time to create more 
connections.

It really is fine-tuning. You can't shield completely from arbitrarily slow 
systems.


tConnection();
 opt.checkConnections();
 }

 private interface Option {

 void acceptConnection();

 void checkConnections() throws InterruptedException;
 }

 private static class OptionA implements Option {

 private final CountDownLatch firstConnection = new CountDownLatch(1);
 private final AtomicInteger connectionsCount = new AtomicInteger();
 private final CountDownLatch otherConnections = new CountDownLatch(1);

 @Override
 public void acceptConnection() {
 if (connectionsCount.incrementAndGet() == 1) {
 firstConnection.countDown();
 } else {
 otherConnections.countDown();
 }
 }

 @Override
 public void checkConnections() throws InterruptedException {
 if (!firstConnection.await(60L, TimeUnit.SECONDS)) {
 throw new RuntimeException("Failed");
 }
 if (otherConnections.await(5L, TimeUnit.SECONDS)) {
 throw new RuntimeException("Unexpected connections: "
+ connectionsCount.get());
 }
 }
 }

 private static class OptionB implements Option {

 private final Semaphore s = new Semaphore(0);

 @Override
 public void acceptConnection() {
 s.release(1);
 }

 @Override
 public void checkConnections() throws InterruptedException {
 if (!s.tryAcquire(60L, TimeUnit.SECONDS)) {
 throw new RuntimeException("Failed");
 }
 if (s.tryAcquire(5L, TimeUnit.SECONDS)) {
 throw new RuntimeException("Unexpected connections: "
+ (s.availablePermits() + 
2));
 }
 }
 }

 private static class OptionC implements Option {

 private final Phaser p = new Phaser(1);

 @Override
 public void acceptConnection() {
 p.arrive();
 }

 @Override
 public void checkConnections() throws 

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

[Chris Yin]

> On 9 Aug 2019, at 1:59 AM, Roger Riggs  wrote:

> 
> ...

> Is handling Unbind in the switch needed (as different from the default).

Sorry, I forgot this, it’s a placeholder to handle UNBIND_REQUEST further, per 
ldap protocol, server will close the connection when received unbind request 
from client (no response required), since the test will shutdown server shortly 
and we don’t have special intention for unbind, so it’s ok to remove unbind in 
switch.

Thanks,
Chris

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

[Pavel Rappo]

Comments are inline.

> On 8 Aug 2019, at 18:59, Roger Riggs  wrote:
> 
> Hi Pavel,
> 
> To your questions...
> 
> 
> On 8/8/19 11:07 AM, Pavel Rappo wrote:
> ...
>>> 109: "template method" doesn't describe the method well, the method is 
>>> private and not overridable.
>>>update the javadoc.
>>> 
>> I can see several questions here. Correct me if I'm wrong. The first one is 
>> about the use of the term "Template Method". This is the name of the 
>> behavioral pattern that has been applied here. We can try to describe what 
>> that method does a bit better, however... here goes the second question. 
>> There is no Javadoc generated for classes like this one. So it's very likely 
>> that the documentation we are talking about will be read in conjunction with 
>> the source code in an editor. Combine that with the fact that this is a test 
>> supporting infrastructure and you might see why I think it would be easier 
>> to just outline the intent of the method. People will read the source code 
>> anyway. Which is not to say that documentation can be avoided altogether, 
>> it's just that I wouldn't sweat it.
> Since its a private method, it is not a template for anything and the term 
> doesn't apply.
>> The third question if that method should be overridable. The canonical 
>> version [1] of the Template Method pattern says: 
>> 
>> "...Primitive operations that _must_ be overridden are declared pure 
>> virtual. The template method itself should not be overridden..."
>> 
> Refering the 'Template" pattern doesn't make the documentation clearer.
>> I don't have a strong opinion about this, only a preference. I prefer that 
>> we will make it overridable only if we find out that the required degree of 
>> extensibility cannot be achieved by introducing additional extension/hook 
>> methods (the ones that the template method calls). Think about it this way. 
>> If that method were overridable, then why would that class be called 
>> `BaseLdapServer`? That method is responsible for that class' "LDAPness"!
> Private is fine.
> 
> ...
>>> 178:  logger() should be final. The subclass has no reason to override.  
>>> And it can be "public final".
>>> 
>> final? Yes. public? I'd rather leave it as it is. I can't think of a case 
>> where an *internal* logger should be sticking out of that class. Hence it's 
>> not public. It might be required when subclassing though, hence it's not 
>> private or of default (package) access.
> final is enough to avoid hacking around without giving it some thought.

I will update the documentation accordingly.

>>> 238: isRunning should be synchronized(lock) to make sure each Thread sees 
>>> the same value threads that update it in start() and close()
>>> 
>> We have already discussed it with you in this thread. The `state` is 
>> `volatile` which is enough for visibility in this query-method.
> yes, but there are multiple locking/synchronization constructs that make the 
> code harder to read
> as unnecessary complexity.

Fine. I will remove `volatile` and synchronize on `lock` in the `isRunning` 
method.

>>> 105:  Tests that sleep are prone to intermittent failures on slow or 
>>> delayed systems.
>>> 
>> True, however, I'm not aware of any general solution of this problem.
> General is not needed, and sleeps waste time time and resources.
> In this particular test, if there is no possiblity of multiple connections 
> then CountDownLatch is a good mechanism.

I would prefer to test for a particular number of connections.

>>> 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.
>>> 
>> Hm... Let me think. What we are interested in here is whether the connection 
>> is attempted or not. We may do what you are suggesting just to be sure the 
>> server is not failing. That is, the client is served successfully.
> The test description is not specific about that.  
> And it raises questions about the purpose of the environment setup.  
> Is it all needed. 

I will update the documentation.

> Is handling Unbind in the switch needed (as different from the default).

Chris might want to answer that.

>>> 
>>> 
>>> If there as some suspecion of too many connections
>>> that could be checked after the beforeConnectionHandled called countDown.
>>> 
>> Wouldn't that make problems of its own? If I got you right, a time window 
>> for the LDAP client to create more *unwanted* connections could be really 
>> small. A hybrid approach might be even better. We wait for the first 
>> connection with a generous timeout and then give the client some extra time 
>> to create more connections.
>> 
>> It really is fine-tuning. You can't shield completely from arbitrarily slow 
>> systems.
>> 
> That's fine, a Semaphore can be used to wait for the first connection and 
> then check with a different timeout for unexpected connections.

I've sketched some possible implementations of the hybrid 

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

[Roger Riggs]

Hi Pavel,

To your questions...


On 8/8/19 11:07 AM, Pavel Rappo wrote:
...

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

I can see several questions here. Correct me if I'm wrong. The first one is about the use 
of the term "Template Method". This is the name of the behavioral pattern that 
has been applied here. We can try to describe what that method does a bit better, 
however... here goes the second question. There is no Javadoc generated for classes like 
this one. So it's very likely that the documentation we are talking about will be read in 
conjunction with the source code in an editor. Combine that with the fact that this is a 
test supporting infrastructure and you might see why I think it would be easier to just 
outline the intent of the method. People will read the source code anyway. Which is not 
to say that documentation can be avoided altogether, it's just that I wouldn't sweat it.
Since its a private method, it is not a template for anything and the 
term doesn't apply.


The third question if that method should be overridable. The canonical version 
[1] of the Template Method pattern says:

"...Primitive operations that _must_ be overridden are declared pure virtual. The 
template method itself should not be overridden..."

Refering the 'Template" pattern doesn't make the documentation clearer.


I don't have a strong opinion about this, only a preference. I prefer that we will make 
it overridable only if we find out that the required degree of extensibility cannot be 
achieved by introducing additional extension/hook methods (the ones that the template 
method calls). Think about it this way. If that method were overridable, then why would 
that class be called `BaseLdapServer`? That method is responsible for that class' 
"LDAPness"!

Private is fine.

...

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

final? Yes. public? I'd rather leave it as it is. I can't think of a case where 
an *internal* logger should be sticking out of that class. Hence it's not 
public. It might be required when subclassing though, hence it's not private or 
of default (package) access.

final is enough to avoid hacking around without giving it some thought.



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

We have already discussed it with you in this thread. The `state` is `volatile` 
which is enough for visibility in this query-method.
yes, but there are multiple locking/synchronization constructs that make 
the code harder to read

as unnecessary complexity.

...

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

True, however, I'm not aware of any general solution of this problem.

General is not needed, and sleeps waste time time and resources.
In this particular test, if there is no possiblity of multiple 
connections then CountDownLatch is a good mechanism.



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.

Hm... Let me think. What we are interested in here is whether the connection is 
attempted or not. We may do what you are suggesting just to be sure the server 
is not failing. That is, the client is served successfully.

The test description is not specific about that.
And it raises questions about the purpose of the environment setup.
Is it all needed.  Is handling Unbind in the switch needed (as different 
from the default).



If there as some suspecion of too many connections
that could be checked after the beforeConnectionHandled called countDown.

Wouldn't that make problems of its own? If I got you right, a time window for 
the LDAP client to create more *unwanted* connections could be really small. A 
hybrid approach might be even better. We wait for the first connection with a 
generous timeout and then give the client some extra time to create more 
connections.

It really is fine-tuning. You can't shield completely from arbitrarily slow 
systems.
That's fine, a Semaphore can be used to wait for the first connection 
and then check with a different timeout for unexpected connections.



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

Pardon, where exactly should we use TestNG? Reconnect.java is a plain 
(psvm-based) test and BaseLdapServer doesn't know anything about TestNG and 
contains no assertions of its own.
TestNG provides a robust set of Assert checks, in many cases they are 
more convenient to write and clear in their semantics compared to 
separate tests and throws.


It is an opportunity, nothing more.

Roger




Thanks for helping with this!

-Pavel

---
[1] Design patterns: elements 

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

[Pavel Rappo]

Roger, answers are inline.

> On 7 Aug 2019, at 16:52, Roger Riggs  wrote:
> 
> 
> 
> BaseLdapServer:
> 
> 100: The new exception should have a message "Unexpected exception" or 
> "server should no be running"...

Fixed.

> 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.

Chris has answered this one.

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

Fixed.

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

I can see several questions here. Correct me if I'm wrong. The first one is 
about the use of the term "Template Method". This is the name of the behavioral 
pattern that has been applied here. We can try to describe what that method 
does a bit better, however... here goes the second question. There is no 
Javadoc generated for classes like this one. So it's very likely that the 
documentation we are talking about will be read in conjunction with the source 
code in an editor. Combine that with the fact that this is a test supporting 
infrastructure and you might see why I think it would be easier to just outline 
the intent of the method. People will read the source code anyway. Which is not 
to say that documentation can be avoided altogether, it's just that I wouldn't 
sweat it.

The third question if that method should be overridable. The canonical version 
[1] of the Template Method pattern says: 

"...Primitive operations that _must_ be overridden are declared pure virtual. 
The template method itself should not be overridden..."

I don't have a strong opinion about this, only a preference. I prefer that we 
will make it overridable only if we find out that the required degree of 
extensibility cannot be achieved by introducing additional extension/hook 
methods (the ones that the template method calls). Think about it this way. If 
that method were overridable, then why would that class be called 
`BaseLdapServer`? That method is responsible for that class' "LDAPness"!

> 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.

Fixed.

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

final? Yes. public? I'd rather leave it as it is. I can't think of a case where 
an *internal* logger should be sticking out of that class. Hence it's not 
public. It might be required when subclassing though, hence it's not private or 
of default (package) access.

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

We have already discussed it with you in this thread. The `state` is `volatile` 
which is enough for visibility in this query-method.

> 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?

Chris has answered those ones. 

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

Fixed.

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

Fixed.

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

True, however, I'm not aware of any general solution of this problem.

> 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.

Hm... Let me think. What we are interested in here is whether the connection is 
attempted or not. We may do what you are suggesting just to be sure the server 
is not failing. That is, the client is served successfully.

> 
> 
> If there as some suspecion of too many connections
> that could be checked after the beforeConnectionHandled called countDown.

Wouldn't that make problems of its own? If I got you right, a time window for 
the LDAP client to create more *unwanted* connections could be really small. A 
hybrid approach might be even better. We wait for the first connection with a 
generous timeout and then give the client some extra time to create more 
connections.

It really is fine-tuning. You can't shield completely from arbitrarily slow 
systems.

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

Pardon, where exactly should we use TestNG? Reconnect.java is a plain 
(psvm-based) test and BaseLdapServer doesn't know anything about TestNG and 
contains no assertions of its own.

Thanks for helping 

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

[Chris Yin]

Many thanks Pavel for the changes, and thanks Lance, Roger for the reviewing.
Yep, let me try to handle some questions from Roger

> On 7 Aug 2019, at 11:52 PM, Roger Riggs  wrote:
> 
> BaseLdapServer:
> 

> 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.

It’s for diagnostic, the ldap server was designed to shadow support ldap tests, 
it should be a black box to ldap client unless there is intent interaction per 
test, so in most case the server side exception should not affect test itself, 
just for diagnostic.
Yep, putting stack traces into log sounds good to me

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

You are correct, it’s a buffer for a single message, so should be singular, 
getMessages() should also be rename to getMessage() to avoid confusion

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

To prevent modification to original LdapMessage, yep, javadoc could be improved 
to make it clear

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

Yes, I believe that should work since we limited the payload length 
presentation <= 4 bytes


Regards,
Chris

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

[Pavel Rappo]

> On 7 Aug 2019, at 18:49, Roger Riggs  wrote:
> 
> 
> 
> It seemed unnecessary/redundant since except for isRunning it is updated 
> inside a synchronize(lock).

Pardon, what seemed unnecessary?

> The synchronized(socketList) is also unnecessary since it is already 
> synchronized on lock.


You are right, this is redundant, albeit harmless. It might be a leftover from
one of the design iterations we've had when synchronization was more
fine-grained. I'll remove all the synchronization on `socketList`.

Thanks.

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

[Roger Riggs]

Hi Pavel,

You are correct, state is volatile.

It seemed unnecessary/redundant since except for isRunning it is updated 
inside a synchronize(lock).
The synchronized(socketList) is also unnecessary since it is already 
synchronized on lock.


Roger


On 8/7/19 1:18 PM, Pavel Rappo wrote:

Roger, thank you for looking at this. While we might hear (on some of your
questions) from Chris soon, I just have to ask about this one


On 7 Aug 2019, at 16:52, Roger Riggs  wrote:

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

What makes you think they can see something different? Even though the result of
this call is immediately outdated, the changes to the state made by start/close
should be propagated as per JMM.

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

[Pavel Rappo]

Roger, thank you for looking at this. While we might hear (on some of your
questions) from Chris soon, I just have to ask about this one

> On 7 Aug 2019, at 16:52, Roger Riggs  wrote:
> 
> 238: isRunning should be synchronized(lock) to make sure each Thread sees the 
> same value threads that update it in start() and close()

What makes you think they can see something different? Even though the result of
this call is immediately outdated, the changes to the state made by start/close
should be propagated as per JMM.

Re: 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  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


  
   

  Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Lance Andersen]

Hi Pavel,

The change looks good.  Nice job on the tests :-)


> On Aug 7, 2019, at 8:47 AM, Pavel Rappo  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
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com