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
