RFR (M): 8231595 [TEST] develop a test case for SuspendThreadList including current thread

[serguei.spitsyn at oracle.com]

Hi Chris and Alex,

I discussed this with Alex before preparing and sending this webrev.
We considered to use a mutex but then I decided that using a raw monitor 
is better.

Thanks,
Serguei


On 10/8/19 18:21, Chris Plummer wrote:
> It's not really being used as a mutex. It's being used as completion 
> flag to ensure proper ordering. That's why I thought it odd.
>
> Chris
>
> On 10/8/19 6:07 PM, Alex Menkov wrote:
>> Hi Chris,
>>
>> Actually this "lock" is used as a mutex.
>> So agent_lock acquires the mutex, agent_unlock releases the mutex.
>>
>> Serguei,
>>
>> The fix looks good to me.
>>
>> Some cosmetic notes (updated webrev is not required):
>>
>> SuspendWithCurrentThread.java
>>
>> 52     public static void main(String args[]) throws Exception {
>>
>> please make it java-style: "String[] args"
>>
>> 79         ThreadToSuspend[]threads = new ThreadToSuspend[threadsCount];
>>
>> missed space: ThreadToSuspend[] threads
>>
>> --alex
>>
>> On 10/08/2019 12:56, Chris Plummer wrote:
>>> Hi Serguei,
>>>
>>> The just seems like an odd coding pattern to me:
>>>
>>>    // Block until the suspender thread competes the tested threads 
>>> suspension
>>>    agent_lock(jni);
>>>    agent_unlock(jni);
>>>
>>> Wouldn't you normally use wait/notify in this situation? Some like:
>>>
>>>    agent_lock(jni);
>>>    if (!suspenderDone) {
>>>      agent_wait(jni);
>>>    }
>>>    agent_unlock(jni);
>>>
>>> I think maybe you left out the middle part because you know that 
>>> this code will never grab the agent_lock before the suspender thread 
>>> does, and therefore once this code gets the lock you know the 
>>> suspender thread is done (is that actually the case?).
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 10/8/19 11:50 AM, serguei.spitsyn at oracle.com wrote:
>>>> Ping...
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 10/7/19 5:25 PM, serguei.spitsyn at oracle.com wrote:
>>>>> On 10/7/19 17:14, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Alex, Chris and David,
>>>>>>
>>>>>> The mach5 testing in 100 runs loop on all platform discovered a 
>>>>>> race in new test.
>>>>>> It is between the native suspendTestedThreads() called on the 
>>>>>> suspender thread
>>>>>> and the checkSuspendedStatus() calling native 
>>>>>> checkTestedThreadsSuspended().
>>>>>>
>>>>>> It occurred that the iteration count and sleep time in 
>>>>>> checkSuspendedStatus() can be not enough:
>>>>>>   100     private boolean checkSuspendedStatus(ThreadToSuspend[] 
>>>>>> threads) throws RuntimeException  {
>>>>>>   101         boolean success = false;
>>>>>>   102
>>>>>>   103         log("Main: checking all tested threads have been 
>>>>>> suspended");
>>>>>>   104
>>>>>>   105         // wait for tested threads to reach suspended status
>>>>>>   106         for (int i = 0; !success && i < 20; i++) {
>>>>>>   107             try {
>>>>>>   108                 Thread.sleep(10);
>>>>>>   109             } catch (InterruptedException e) {
>>>>>>   110               throw new RuntimeException("Main: sleep was 
>>>>>> interrupted\n\t" + e);
>>>>>>   111             }
>>>>>>   112             success = checkTestedThreadsSuspended();
>>>>>>   113         }
>>>>>>   114         return success;
>>>>>>   115     }
>>>>>>
>>>>>> So, I've decided to refactor/update the test a little bit:
>>>>>>
>>>>>> 1. Now the checkSuspendedStatus() is:
>>>>>>   100     private boolean checkSuspendedStatus() throws 
>>>>>> RuntimeException  {
>>>>>>   101         log("Main: checking all tested threads have been 
>>>>>> suspended");
>>>>>>   102         return checkTestedThreadsSuspended();
>>>>>>   103     }
>>>>>> 2. A raw monitor agent_monitor is added to the native agent.
>>>>>>    It is created and locked in the Java_ThreadToSuspend_init() 
>>>>>> function called at
>>>>>>    the beginning of the ThreadToSuspend.run() execution of the 
>>>>>> suspender thread
>>>>>>    and unlocked at the end of 
>>>>>> Java_ThreadToSuspend_suspendTestedThreads().
>>>>>>
>>>>>>    The 
>>>>>> Java_SuspendWithCurrentThread_checkTestedThreadsSuspended() grabs 
>>>>>> the agent_monitor
>>>>>>    on the Main thread to wait until the suspendTestedThreads() 
>>>>>> completes its work.
>>>>>>
>>>>>> 3. Also, the results[] array is always created and used locally 
>>>>>> in the functions
>>>>>>    where it is needed: suspendTestedThreads() and 
>>>>>> resumeTestedThreads().
>>>>>
>>>>> Forgot to tell about one more change:
>>>>>
>>>>> 4. The releaseTestedThreads() is renamed to 
>>>>> releaseTestedThreadsInfo() and moved
>>>>>    to the end of the Main thread run() method.
>>>>>
>>>>> Also wanted to thank Alex for sharing good suggestions on the sync 
>>>>> approaches.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>>
>>>>>> Updated webrev version:
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.4/ 
>>>>>>
>>>>>>
>>>>>>
>>>>>> Testing:
>>>>>>   One mach5 run with 100 rep counter successfully passed.
>>>>>>   To be more confident, I've submitted one more mach5 job which 
>>>>>> is in progress.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>
>>>>
>>>
>
>