RFR [6968459] JNDI timeout fails before timeout is reached

[Peter Levart]

On 11/29/2013 09:06 PM, Ivan Gerasimov wrote:
> Thank you Alan for the reply!
>
> On 29.11.2013 21:03, Alan Bateman wrote:
>> On 19/11/2013 17:58, Ivan Gerasimov wrote:
>>> Hello all!
>>>
>>> Would you please help review a fix for the bug?
>>> https://bugs.openjdk.java.net/browse/JDK-6968459
>>>
>>> It was reported that creating new InitialLdapContext() can fail with 
>>> "javax.naming.NamingException: LDAP response read timed out, timeout 
>>> used:30000ms", even though the specified timeout hadn't been elapsed.
>>>
>>> The fix was provided by the filer of the bug some time ago.
>>> Here's the webrev with this fix:
>>> http://cr.openjdk.java.net/~igerasim/6968459/0/webrev/
>>
>> I haven't seen any replies to this but I've cc'ed Vinnie and Xuelei 
>> as they are more familiar with this area.
>>
>> If I understand correctly then the issue is that the timeout handling 
>> doesn't take account of wakeups when aren't any BerDecoders to 
>> dequeue. The changes mean it will retry the wait with a decreasing 
>> timeout until a reply is received or the timeout elapses. That seems 
>> reasonable, assuming the time doesn't change :-)   You might find the 
>> code is a bit clearer if you have a "remaining" time as that would 
>> allow you get rid of timedOut, timeOut and endTime.
>>
> I modified the patch in the way you suggest.
> http://cr.openjdk.java.net/~igerasim/6968459/1/webrev/
>
> The timeOut variable now holds the remaining time.
> If the system time had changed back, we start counting from the 
> beginning.
> If it had changed forward, we have no way to catch it and the timeout 
> gets elapsed earlier.
>
>> I see the patch doesn't come with a test. Is there any test 
>> infrastructure for testing LDAP without require a complete server?
>>
> I didn't find anything like that, that's why I set 'noreg-hard' label.
>
> Sincerely yours,
> Ivan
>
>
>> -Alan.
>>
>>
>

Hi Ivan,

 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