Review Request: 7196045 Possible JVM deadlock in ThreadTimesClosure

[serguei.spitsyn at oracle.com]

It looks good.
As far as I understand allocating memory in ResourceArena (with Amalloc) 
should not cause GC.

Thanks,
Serguei

On 9/5/12 9:26 AM, Kevin Walls wrote:
> Hi again,
>
> I found a memory leak while testing this.  thread->name() uses the 
> ResourceArea.  I don't think we've been able to call this enough in 
> the past to notice the leak, but now we can...  Adding a ResourceMark 
> (management.cpp line 1859) there's no leak.
>
> I updated the webrev in the same place: 
> http://cr.openjdk.java.net/~kevinw/7196045/webrev.01/
>
> Thanks
> Kevin
>
>
> On 05/09/2012 11:34, David Holmes wrote:
>> On 5/09/2012 7:16 PM, Kevin Walls wrote:
>>>
>>> Hi David,
>>>
>>> We could allocate some tens of kb... more if there are several thousand
>>> threads with very long names... It's not a commonly called method, but
>>> that's why the deadlock hasn't been exposed before. Yes you'd need 
>>> to be
>>> very near that existing cliff-edge...
>>>
>>>
>>> Just thinking that yes if we are low on memory and don't die from the
>>> NEW_C_HEAP_ARRAY, then the strdup could fail. I'll add a check that we
>>> have something to free() in case that fails on some platforms. In the
>>> loop below if _names_chars[i] was null, the app will get a null string,
>>> again probably the least of it's worries at that point.
>>
>> It's okay to call free(NULL) so no need to check for it explicitly. 
>> And the NULL is handled by create_from_str.
>>
>> But the CHECK macro will cause us to return if an exception is posted 
>> and we will leak the dup'd string in that case.
>>
>> David
>>
>>> Thanks
>>> Kevin
>>>
>>>
>>> for (int i=0; i<_count; i++) {
>>> Handle s = java_lang_String::create_from_str(_names_chars[i], CHECK);
>>> _names_strings->obj_at_put(i, s());
>>> if (_names_chars[i] != NULL) {
>>> free(_names_chars[i]);
>>> }
>>> }
>>>
>>>
>>>
>>> On 05/09/12 02:51, David Holmes wrote:
>>>> Hi Kevin,
>>>>
>>>> On 5/09/2012 8:35 AM, Kevin Walls wrote:
>>>>> I'd like to get this reviewed, it's a deadlock that can happen due to
>>>>> Java objects being allocated while holding Threads_lock.
>>>>>
>>>>> We need to collect just the characters of the thread names while 
>>>>> holding
>>>>> the lock, and create the String objects when it's released. You do 
>>>>> need
>>>>> to hit the (non-public) HotspotInternal MBean very rapidly to trigger
>>>>> this reliably, but there's a small chance the deadlock could 
>>>>> happen when
>>>>> it's called by the tool that is meant to call it.
>>>>>
>>>>> http://cr.openjdk.java.net/~kevinw/7196045/webrev.01/
>>>>
>>>> The only thing I don't like here is that the use of NEW_C_HEAP_ARRAY
>>>> will cause an abort on out-of-memory and I'm not sure how big this
>>>> array might be. If you are running out of C-Heap then you're on the
>>>> edge of the cliff anyway and likely to fall at any minute, but I still
>>>> dislike seeing more cases added to the code. I don't see an
>>>> alternative though. :(
>>>>
>>>> David
>>>>
>>>>
>>>>> Thanks!
>>>>> Kevin
>>>>>
>>>