Review Request: 7196045 Possible JVM deadlock in ThreadTimesClosure

[Kevin Walls]

Ah yes, so doing all the cleaning up in the destructor has to be the way 
to do it:

Thanks
Kevin

// Called without Threads_lock, we can allocate String objects.
void ThreadTimesClosure::do_unlocked() {

   EXCEPTION_MARK;
   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());
   }
}

ThreadTimesClosure::~ThreadTimesClosure() {
   for (int i=0; i<_count; i++) {
     free(_names_chars[i]);
   }
   FREE_C_HEAP_ARRAY(char *, _names_chars, mtInternal);
}



On 05/09/12 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
>>>>
>>