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.mcke...@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
> > > >
> > >
>
