RFR [6968459] JNDI timeout fails before timeout is reached

Peter Levart peter.levart at gmail.com
Tue Dec 3 16:05:31 UTC 2013


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




More information about the core-libs-dev mailing list