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

[Rob McKenna]

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