RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize
Daniel Fuchs
daniel.fuchs at oracle.com
Wed Sep 5 08:53:47 UTC 2018
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.mckenna at 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
>>>
>>
More information about the core-libs-dev
mailing list