Hi Peter!
Thank you for your review!
You are right, the patch changed the behavior of the code.
I've reverted back all the unnecessary changes. This should minimize the
risk.
I've also made another correction: After decrementing the remaining
timeOut, the startTime should be set to currTime.
Would you please help review the updated webrev:
http://cr.openjdk.java.net/~igerasim/6968459/2/webrev/
Sincerely yours,
Ivan
From quick look I notice a danger that Connection.readReply() pauses
(for the timeOut time) in spite of the fact that a reply is ready and
enqueued before waiting.
Imagine a situation where the 1st try of obtaining result returns null:
450 // Get the reply if it is already available.
451 BerDecoder rber = ldr.getReplyBer();
...after that, but before reaching the following lines:
471 synchronized (ldr) {
472 ldr.wait(timeOut);
473 }
The "other" thread enqueues a reply (LdapRequest.addReplyBer())
because the code in readReply() is executing without holding a lock on
"ldr". When "this" thread (executing readReply()) finally enters
synchronized block and waits, it will wait until wait() times out or
another reply is enqueued which will issue another notify(). This can
lead to unnecessary pauses. Old code does not exhibit this problem,
because it re-checks the readiness of a reply immediately after
entering the synchronized block.
Regards, Peter
