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

[Alex Menkov]

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