On 11/29/2013 09:06 PM, Ivan Gerasimov wrote:
Thank you Alan for the reply!
On 29.11.2013 21:03, Alan Bateman wrote:
On 19/11/2013 17:58, Ivan Gerasimov wrote:
Hello all!
Would you please help review a fix for the bug?
https://bugs.openjdk.java.net/browse/JDK-6968459
It was reported that creating new InitialLdapContext() can fail with
"javax.naming.NamingException: LDAP response read timed out, timeout
used:30000ms", even though the specified timeout hadn't been elapsed.
The fix was provided by the filer of the bug some time ago.
Here's the webrev with this fix:
http://cr.openjdk.java.net/~igerasim/6968459/0/webrev/
I haven't seen any replies to this but I've cc'ed Vinnie and Xuelei
as they are more familiar with this area.
If I understand correctly then the issue is that the timeout handling
doesn't take account of wakeups when aren't any BerDecoders to
dequeue. The changes mean it will retry the wait with a decreasing
timeout until a reply is received or the timeout elapses. That seems
reasonable, assuming the time doesn't change :-) You might find the
code is a bit clearer if you have a "remaining" time as that would
allow you get rid of timedOut, timeOut and endTime.
I modified the patch in the way you suggest.
http://cr.openjdk.java.net/~igerasim/6968459/1/webrev/
The timeOut variable now holds the remaining time.
If the system time had changed back, we start counting from the
beginning.
If it had changed forward, we have no way to catch it and the timeout
gets elapsed earlier.
I see the patch doesn't come with a test. Is there any test
infrastructure for testing LDAP without require a complete server?
I didn't find anything like that, that's why I set 'noreg-hard' label.
Sincerely yours,
Ivan
-Alan.
Hi 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
