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

[Daniel Fuchs]

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
>