RFR [6968459] JNDI timeout fails before timeout is reached

[David Holmes]

If you track the elapsed waiting time using System.nanoTime you do not 
need to be concerned whether anyone messes with the TOD clock.

David

On 4/12/2013 2:05 AM, Peter Levart wrote:
> On 12/03/2013 03:35 PM, Ivan Gerasimov wrote:
>> Hi Peter!
>>
>> Thank you for your review!
>>
>> You are right, the patch changed the behavior of the code.
>> I've reverted back all the unnecessary changes. This should minimize
>> the risk.
>>
>> I've also made another correction: After decrementing the remaining
>> timeOut, the startTime should be set to currTime.
>>
>> Would you please help review the updated webrev:
>> http://cr.openjdk.java.net/~igerasim/6968459/2/webrev/
>>
>> Sincerely yours,
>> Ivan
>
> Hi Ivan,
>
> That's better. You could move the initial request for ldr.getReplyBer()
> (line 447) out of while loop, since it is not needed in the loop (all
> further ldr.getReplyBer() calls in loop are performed in synchronized
> block already - line 465). "else { break; }" (line 469) is not needed in
> that case. I would also make timeOut variable long to avoid overflows.
> Simplified logic could look like this:
>
>
>      BerDecoder readReply(LdapRequest ldr)
>              throws IOException, NamingException {
>          long timeOut = (readTimeout > 0) ?
>                  readTimeout : 15 * 1000; // Default read timeout of 15 sec
>
>          long currTime = System.currentTimeMillis();
>
>          BerDecoder rber = ldr.getReplyBer();
>
>          while (rber == null && timeOut > 0L) {
>              long startTime = currTime;
>              // If socket closed, don't even try
>              synchronized (this) {
>                  if (sock == null) {
>                      throw new ServiceUnavailableException(host + ":" +
> port +
> "; socket closed");
>                  }
>              }
>              synchronized (ldr) {
>                  // check if condition has changed since our last check
>                  rber = ldr.getReplyBer();
>                  if (rber == null) {
>                      try {
>                          ldr.wait(timeOut);
>                      } catch (InterruptedException ex) {
>                          throw new InterruptedNamingException(
>                              "Interrupted during LDAP operation");
>                      }
>                  }
>              }
>              currTime = System.currentTimeMillis();
>              if (startTime < currTime) {
>                  timeOut -= (currTime - startTime);
>              }
>              // else system time must have changed backwards (or not at
> all)
>          }
>
>          if (rber == null && readTimeout > 0) {
>              removeRequest(ldr);
>              throw new NamingException("LDAP response read timed out,
> timeout used:"
>                              + readTimeout + "ms." );
>          }
>
>          return rber;
>      }
>
>
> What do you think?
>
> Regards, Peter
>
>>
>>>
>>> From quick look I notice a danger that Connection.readReply() pauses
>>> (for the timeOut time) in spite of the fact that a reply is ready and
>>> enqueued before waiting.
>>>
>>> Imagine a situation where the 1st try of obtaining result returns null:
>>>
>>>  450         // Get the reply if it is already available.
>>>  451         BerDecoder rber = ldr.getReplyBer();
>>>
>>>
>>> ...after that, but before reaching the following lines:
>>>
>>>  471                     synchronized (ldr) {
>>>  472                         ldr.wait(timeOut);
>>>  473                     }
>>>
>>>
>>> The "other" thread enqueues a reply (LdapRequest.addReplyBer())
>>> because the code in readReply() is executing without holding a lock
>>> on "ldr". When "this" thread (executing readReply()) finally enters
>>> synchronized block and waits, it will wait until wait() times out or
>>> another reply is enqueued which will issue another notify(). This can
>>> lead to unnecessary pauses. Old code does not exhibit this problem,
>>> because it re-checks the readiness of a reply immediately after
>>> entering the synchronized block.
>>>
>>>
>>> Regards, Peter
>>>
>>>
>>>
>>
>