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