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 <Roger.Riggs at oracle.com> 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.
>>>> <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.
>>>
> 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 InterruptedException {
>              try {
>                  p.awaitAdvanceInterruptibly(0, 60L, TimeUnit.SECONDS);
>              } catch (TimeoutException e) {
>                  throw new RuntimeException("Failed");
>              }
>              try {
>                  p.awaitAdvanceInterruptibly(1, 5L, TimeUnit.SECONDS);
>              } catch (TimeoutException e) {
>                  return; /* expected */
>              }
>              throw new RuntimeException("Unexpected connections: "
>                                                 + p.getPhase());
>          }
>      }
> }
>
> // --------------------------------------------------
>
>