Hello Ivan, Thanks for the updated patch. I would like to see a testcase along with this fix, since it is modifying a critical component of the LDAP client code. An LDAP server may not even be required in order to exercise the timeouts.
Thanks. On 3 Dec 2013, at 14:35, Ivan Gerasimov <ivan.gerasi...@oracle.com> 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 > >> >> 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 >> >> >> >
