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
