RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Daniel Fuchs]

Hi Rob,

Now I'm concerned about this method:

   71     synchronized boolean addReplyBer(BerDecoder ber) {
   72         // check the closed boolean value here as we don't want 
anything
   73         // to be added to the queue after close() has been called.
   74         if (cancelled || closed) {
   75             return false;
   76         }
   77
   78         // Add a new reply to the queue of unprocessed replies.
   79         try {
   80             replies.put(ber);
   81         } catch (InterruptedException e) {
   82             // ignore
   83         }
   84
   85         // peek at the BER buffer to check if it is a 
SearchResultDone PDU
   86         try {
   87             ber.parseSeq(null);
   88             ber.parseInt();
   89             completed = (ber.peekByte() == 
LdapClient.LDAP_REP_RESULT);
   90         } catch (IOException e) {
   91             // ignore
   92         }
   93         ber.reset();
   94         return pauseAfterReceipt;
   95     }

getReplyBer() is not synchronized, so AFAICS there is a
chance that line 93 executes after another thread as got
hold of the `ber` object.

Is that an issue? Should the `ber` object be added to
the reply queue only after it's been reset?

best regards,

-- daniel

On 25/10/2018 21:53, Rob McKenna wrote:
> Thanks Daniel:
> 
> http://cr.openjdk.java.net/~robm/8139965/webrev.03/
> 
> I'm planning to follow up on the test side of things with a separate
> bug. I think the technique used in some of the recent SQE LDAP tests
> might be applicable.
> 
>      -Rob
> 
> On 05/09/18 09:53, Daniel Fuchs wrote:
>> Hi Rob,
>>
>> That looks better but I believe that:
>>
>> 1. closed should be volatile since it's read from outside
>>     synchronized block
>>
>> 2. it seems there might be a race where the last response
>>     received could be dropped, if the connection is closed
>>     just after it's been pulled from the queue.
>>
>> So I would suggest exchanging:
>>
>>   115         if (isClosed()) {
>>   116             return null;
>>   117         }
>>   118
>>   119         return result;
>>
>> with:
>>
>>               return result == EOF ? null : result;
>>
>> best regards,
>>
>> -- daniel
>>
>> On 05/09/2018 02:05, Rob McKenna wrote:
>>> Thanks for the reviews folks.
>>>
>>> I believe the following captures your recommended changes:
>>>
>>> http://cr.openjdk.java.net/~robm/8139965/webrev.02/
>>>
>>> W.r.t. testing I think this area has been difficult to test
>>> traditionally. I'll have a dig through the existing testbase (and I'll
>>> get back to you) to see if there's anything similar but afaik most tests
>>> simply mimic a bindable dummy ldap server.
>>>
>>> Vyom, are you aware of any more rigorous tests / ldap test frameworks?
>>>
>>>       -Rob
>>>
>>> On 04/09/18 10:22, Daniel Fuchs wrote:
>>>> Hi Rob,
>>>>
>>>> I concur with Chris.
>>>> completed needs to be volatile and close() needs to
>>>> set a flag and use offer like cancel().
>>>>
>>>> The condition for testing for closed then becomes
>>>> that the flag is set and the queue is empty or has EOF
>>>> as its head.
>>>>
>>>> Is there any way this could be tested by a regression
>>>> test?
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>> On 04/09/2018 10:00, Chris Hegarty wrote:
>>>>> Rob,
>>>>>
>>>>>> On 3 Sep 2018, at 22:48, Rob McKenna <rob.mckenna at oracle.com> wrote:
>>>>>>
>>>>>> Hi folks,
>>>>>>
>>>>>> I'd like to get a re-review of this change:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8139965 <https://bugs.openjdk.java.net/browse/JDK-8139965>
>>>>>
>>>>> This issue is closed as `will not fix`. I presume you will re-open it before pushing.
>>>>>
>>>>>> http://cr.openjdk.java.net/~robm/8139965/webrev/ <http://cr.openjdk.java.net/~robm/8139965/webrev/>
>>>>>
>>>>>
>>>>> 43     private boolean completed;
>>>>> Won’t `completed` need to be volatile now? ( since the removal of synchronized from hasSearchCompleted )
>>>>>
>>>>> LdapRequest.close puts EOF into the queue, but that is a potentially blocking operation ( while holding the lock ). If the queue is at capacity, then it will block forever. This model will only work if `getReplyBer` is always guaranteed to be running concurrently. Is it?
>>>>>
>>>>> Please check the indentation of LdapRequest.java L103 ( in
>>>>> the new file ). It appears, in the webrev, that the trailing `}` is
>>>>> not lined up.
>>>>>
>>>>> The indentation doesn’t look right here either.
>>>>>     624             if (nparent) {
>>>>>     625                 LdapRequest ldr = pendingRequests;
>>>>>     626                 while (ldr != null) {
>>>>>     627                     ldr.close();
>>>>>     628                         ldr = ldr.next;
>>>>>     629                     }
>>>>>     630                 }
>>>>>     631             }
>>>>>
>>>>> -Chris
>>>>>
>>>>
>>