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

[Chris Plummer]

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
>>>>
>>>
>>