jmx-dev RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

jmx-dev RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

Shanliang JIANG
Hi,

Please review this fix:

bug: https://bugs.openjdk.java.net/browse/JDK-8068418
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/

This must be a timing issue in the test, the test called:
    t.join(5000L);  
to wait a thread's dying,  I reproduced the bug by insert at line 202:
    try {
        Thread.sleep(5100);
    } catch (Exception ee) {}
to delay the t's dying.

The fix is to replace:
    t.join(5000L);
by:
    t.join();

and replace:
    Object.wait(timeout);
by
    CountDownLatch.countDown();

The test harness timeout will be used as the max waiting timeout.

Shanliang
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

David Holmes
Hi Shanliang,

On 6/01/2015 3:47 AM, shanliang wrote:

> Hi,
>
> Please review this fix:
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8068418
> webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/
>
> This must be a timing issue in the test, the test called:
>     t.join(5000L); to wait a thread's dying,  I reproduced the bug by
> insert at line 202:
>     try {
>         Thread.sleep(5100);
>     } catch (Exception ee) {}
> to delay the t's dying.
>
> The fix is to replace:
>     t.join(5000L);
> by:
>     t.join();
>
> and replace:
>     Object.wait(timeout);
> by
>     CountDownLatch.countDown();

Okay - you could have just done wait() but the switch to CountDownLatch
is somewhat cleaner.

> The test harness timeout will be used as the max waiting timeout.

So the test will now never report that it thinks it has hit a deadlock,
it will just timeout. But you added some extra printout so okay I guess
- but I suggest adding a comment at the top (where @run etc are) saying
that if test times out then deadlock is suspected.

Minor nit:

  System.out.println("DeadlockTest-addNotificationListener waiting the
XXX thread to die.

should say "waiting for the XXX thread ..."

Thanks,
David

> Shanliang
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

Shanliang JIANG
David,

Here is the new version, I have added the comments as you suggested, and
I replaced the variable "sources" with a synchronized list.
    http://cr.openjdk.java.net/~sjiang/JDK-8068418/01/

Thanks for the review.
Shanliang

David Holmes wrote:

> Hi Shanliang,
>
> On 6/01/2015 3:47 AM, shanliang wrote:
>> Hi,
>>
>> Please review this fix:
>>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8068418
>> webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/
>>
>> This must be a timing issue in the test, the test called:
>>     t.join(5000L); to wait a thread's dying,  I reproduced the bug by
>> insert at line 202:
>>     try {
>>         Thread.sleep(5100);
>>     } catch (Exception ee) {}
>> to delay the t's dying.
>>
>> The fix is to replace:
>>     t.join(5000L);
>> by:
>>     t.join();
>>
>> and replace:
>>     Object.wait(timeout);
>> by
>>     CountDownLatch.countDown();
>
> Okay - you could have just done wait() but the switch to
> CountDownLatch is somewhat cleaner.
>
>> The test harness timeout will be used as the max waiting timeout.
>
> So the test will now never report that it thinks it has hit a
> deadlock, it will just timeout. But you added some extra printout so
> okay I guess - but I suggest adding a comment at the top (where @run
> etc are) saying that if test times out then deadlock is suspected.
>
> Minor nit:
>
>  System.out.println("DeadlockTest-addNotificationListener waiting the
> XXX thread to die.
>
> should say "waiting for the XXX thread ..."
>
> Thanks,
> David
>
>> Shanliang

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

David Holmes
On 6/01/2015 9:03 PM, shanliang wrote:
> David,
>
> Here is the new version, I have added the comments as you suggested, and
> I replaced the variable "sources" with a synchronized list.

Why did you do that ?? Vector is synchronized so it was fine for the job.

David

>     http://cr.openjdk.java.net/~sjiang/JDK-8068418/01/
>
> Thanks for the review.
> Shanliang
>
> David Holmes wrote:
>> Hi Shanliang,
>>
>> On 6/01/2015 3:47 AM, shanliang wrote:
>>> Hi,
>>>
>>> Please review this fix:
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8068418
>>> webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/
>>>
>>> This must be a timing issue in the test, the test called:
>>>     t.join(5000L); to wait a thread's dying,  I reproduced the bug by
>>> insert at line 202:
>>>     try {
>>>         Thread.sleep(5100);
>>>     } catch (Exception ee) {}
>>> to delay the t's dying.
>>>
>>> The fix is to replace:
>>>     t.join(5000L);
>>> by:
>>>     t.join();
>>>
>>> and replace:
>>>     Object.wait(timeout);
>>> by
>>>     CountDownLatch.countDown();
>>
>> Okay - you could have just done wait() but the switch to
>> CountDownLatch is somewhat cleaner.
>>
>>> The test harness timeout will be used as the max waiting timeout.
>>
>> So the test will now never report that it thinks it has hit a
>> deadlock, it will just timeout. But you added some extra printout so
>> okay I guess - but I suggest adding a comment at the top (where @run
>> etc are) saying that if test times out then deadlock is suspected.
>>
>> Minor nit:
>>
>>  System.out.println("DeadlockTest-addNotificationListener waiting the
>> XXX thread to die.
>>
>> should say "waiting for the XXX thread ..."
>>
>> Thanks,
>> David
>>
>>> Shanliang
>
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

Shanliang JIANG
David Holmes wrote:
> On 6/01/2015 9:03 PM, shanliang wrote:
>> David,
>>
>> Here is the new version, I have added the comments as you suggested, and
>> I replaced the variable "sources" with a synchronized list.
>
> Why did you do that ?? Vector is synchronized so it was fine for the job.
You are right! I have changed back to Vector:
    http://cr.openjdk.java.net/~sjiang/JDK-8068418/02/

I forgot Vetor is synchronized, so long time not use it.

Thanks,
Shanliang

>
> David
>
>>     http://cr.openjdk.java.net/~sjiang/JDK-8068418/01/
>>
>> Thanks for the review.
>> Shanliang
>>
>> David Holmes wrote:
>>> Hi Shanliang,
>>>
>>> On 6/01/2015 3:47 AM, shanliang wrote:
>>>> Hi,
>>>>
>>>> Please review this fix:
>>>>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8068418
>>>> webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/
>>>>
>>>> This must be a timing issue in the test, the test called:
>>>>     t.join(5000L); to wait a thread's dying,  I reproduced the bug by
>>>> insert at line 202:
>>>>     try {
>>>>         Thread.sleep(5100);
>>>>     } catch (Exception ee) {}
>>>> to delay the t's dying.
>>>>
>>>> The fix is to replace:
>>>>     t.join(5000L);
>>>> by:
>>>>     t.join();
>>>>
>>>> and replace:
>>>>     Object.wait(timeout);
>>>> by
>>>>     CountDownLatch.countDown();
>>>
>>> Okay - you could have just done wait() but the switch to
>>> CountDownLatch is somewhat cleaner.
>>>
>>>> The test harness timeout will be used as the max waiting timeout.
>>>
>>> So the test will now never report that it thinks it has hit a
>>> deadlock, it will just timeout. But you added some extra printout so
>>> okay I guess - but I suggest adding a comment at the top (where @run
>>> etc are) saying that if test times out then deadlock is suspected.
>>>
>>> Minor nit:
>>>
>>>  System.out.println("DeadlockTest-addNotificationListener waiting the
>>> XXX thread to die.
>>>
>>> should say "waiting for the XXX thread ..."
>>>
>>> Thanks,
>>> David
>>>
>>>> Shanliang
>>

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

David Holmes
On 6/01/2015 10:56 PM, shanliang wrote:

> David Holmes wrote:
>> On 6/01/2015 9:03 PM, shanliang wrote:
>>> David,
>>>
>>> Here is the new version, I have added the comments as you suggested, and
>>> I replaced the variable "sources" with a synchronized list.
>>
>> Why did you do that ?? Vector is synchronized so it was fine for the job.
> You are right! I have changed back to Vector:
>     http://cr.openjdk.java.net/~sjiang/JDK-8068418/02/
>
> I forgot Vetor is synchronized, so long time not use it.

:) Thanks - seems okay to me.

David


> Thanks,
> Shanliang
>>
>> David
>>
>>>     http://cr.openjdk.java.net/~sjiang/JDK-8068418/01/
>>>
>>> Thanks for the review.
>>> Shanliang
>>>
>>> David Holmes wrote:
>>>> Hi Shanliang,
>>>>
>>>> On 6/01/2015 3:47 AM, shanliang wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this fix:
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8068418
>>>>> webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/
>>>>>
>>>>> This must be a timing issue in the test, the test called:
>>>>>     t.join(5000L); to wait a thread's dying,  I reproduced the bug by
>>>>> insert at line 202:
>>>>>     try {
>>>>>         Thread.sleep(5100);
>>>>>     } catch (Exception ee) {}
>>>>> to delay the t's dying.
>>>>>
>>>>> The fix is to replace:
>>>>>     t.join(5000L);
>>>>> by:
>>>>>     t.join();
>>>>>
>>>>> and replace:
>>>>>     Object.wait(timeout);
>>>>> by
>>>>>     CountDownLatch.countDown();
>>>>
>>>> Okay - you could have just done wait() but the switch to
>>>> CountDownLatch is somewhat cleaner.
>>>>
>>>>> The test harness timeout will be used as the max waiting timeout.
>>>>
>>>> So the test will now never report that it thinks it has hit a
>>>> deadlock, it will just timeout. But you added some extra printout so
>>>> okay I guess - but I suggest adding a comment at the top (where @run
>>>> etc are) saying that if test times out then deadlock is suspected.
>>>>
>>>> Minor nit:
>>>>
>>>>  System.out.println("DeadlockTest-addNotificationListener waiting the
>>>> XXX thread to die.
>>>>
>>>> should say "waiting for the XXX thread ..."
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Shanliang
>>>
>