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

[Rob McKenna]

Thanks again Daniel,

http://cr.openjdk.java.net/~robm/8139965/webrev.04/

    -Rob

On 26/10/18 09:54, Daniel Fuchs wrote:
> 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
> > > > > > 
> > > > > 
> > > 
>