On 12/03/2013 03:35 PM, Ivan Gerasimov wrote:
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
Hi Ivan,
That's better. You could move the initial request for ldr.getReplyBer()
(line 447) out of while loop, since it is not needed in the loop (all
further ldr.getReplyBer() calls in loop are performed in synchronized
block already - line 465). "else { break; }" (line 469) is not needed in
that case. I would also make timeOut variable long to avoid overflows.
Simplified logic could look like this:
BerDecoder readReply(LdapRequest ldr)
throws IOException, NamingException {
long timeOut = (readTimeout > 0) ?
readTimeout : 15 * 1000; // Default read timeout of 15 sec
long currTime = System.currentTimeMillis();
BerDecoder rber = ldr.getReplyBer();
while (rber == null && timeOut > 0L) {
long startTime = currTime;
// If socket closed, don't even try
synchronized (this) {
if (sock == null) {
throw new ServiceUnavailableException(host + ":" +
port +
"; socket closed");
}
}
synchronized (ldr) {
// check if condition has changed since our last check
rber = ldr.getReplyBer();
if (rber == null) {
try {
ldr.wait(timeOut);
} catch (InterruptedException ex) {
throw new InterruptedNamingException(
"Interrupted during LDAP operation");
}
}
}
currTime = System.currentTimeMillis();
if (startTime < currTime) {
timeOut -= (currTime - startTime);
}
// else system time must have changed backwards (or not at all)
}
if (rber == null && readTimeout > 0) {
removeRequest(ldr);
throw new NamingException("LDAP response read timed out,
timeout used:"
+ readTimeout + "ms." );
}
return rber;
}
What do you think?
Regards, Peter
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