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 <Roger.Riggs at oracle.com> wrote:
> 
> <snip>
> 
> 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.

> <snip>
> 
> 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 with this!

-Pavel

---------------------------------------------------
[1] Design patterns: elements of reusable object-oriented software (ISBN-13: 978-0201633610)