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