RFR: JDK-6545967: sp05t003 failed ResumeThread() due to THREAD_NOT_SUSPENDED

[serguei.spitsyn at oracle.com]

Hi Gary,

Looks great modulo the suggestion from Chris.

Thanks,
Serguei


On 6/20/18 10:45, Chris Plummer wrote:
> Hi Gary,
>
> Overall looks good. One minor formatting comment:
>
> 105 if ((mt.threadState < 1000) &&
> 106 agentStatus()) {
>  107             passed = true;
>  108         }
>
>
> Lines 105 and 106 should be combined.
>
> thanks
>
> Chris
>
> On 6/20/18 5:31 AM, Gary Adams wrote:
>> We do need to consider an early return for the case where
>> the redefine class or suspend thread operation have detected
>> an error.
>>
>>
>> Webrev:http://bussund0416.us.oracle.com/export/users/gradams/work/webrevs/6545967/webrev.01/
>>
>>
>>    80     public boolean agentMethod() {
>>    81         boolean passed = false;
>>    82         MyThread mt = new MyThread();
>>    83         try {
>>    84             mt.start();
>>    85             // check if we can can pop the thread.
>> 86 // we can not do redefine/pop frame on run method.
>> 87 while (!MyThread.resume.get());
>>
>>    88             // sleep for some few secs to get redefined.
>> 89 while (!isRedefined()) {
>> 90 if (!agentStatus()) {
>> 91 System.out.println("Failed to redefine class");
>> 92 return passed;
>> 93 }
>> 94 Thread.sleep(100);
>> 95 }
>> 96 popThreadFrame(mt); // pop the frame.
>> 97 resumeThread(mt); // resume the thread.
>>    98             mt.join();
>>    99             // wait till the other thread completes its execution.
>> 100 System.out.println("Thread state after popping/redefining = "
>> 101 + mt.threadState);
>>   102         } catch(Exception ie) {
>>   103             ie.printStackTrace();
>>   104         }
>> 105 if ((mt.threadState < 1000) &&
>> 106 agentStatus()) {
>>
>>   107             passed = true;
>>   108         }
>>   109         return passed;
>>   110     }
>>
>>
>>
>> On 6/19/18, 7:53 PM, Chris Plummer wrote:
>>> Hi Gary,
>>>
>>> The sp05t003 changes look fine. Serquei's question about hs203t003 
>>> leads me to ask what happens if the redefine never happens, possibly 
>>> due to some error. Seems we are destined for a timeout then (not 
>>> sure how long that will take), whereas the current implementation 
>>> would report the failure after about 10 seconds.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 6/19/18 2:26 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Gary,
>>>>
>>>> The fix looks good in general.
>>>>
>>>> One comment though:
>>>>
>>>> http://cr.openjdk.java.net/~gadams/6545967/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS203/hs203t003/hs203t003.java.frames.html 
>>>>
>>>>
>>>>    80     public boolean  agentMethod() {
>>>>    81         boolean passed = false;
>>>>    82         MyThread mt = new MyThread();
>>>>    83                 try {
>>>>    84             mt.start();
>>>>    85             // check if we can can pop the thread.
>>>>    86             // we can not do redefine / pop frame on run method.
>>>>    87             while(!MyThread.resume.get());
>>>>    88             // sleep for some few secs to get redefined.
>>>> 89 while(!isRedefined()) {
>>>> 90 Thread.sleep(100);
>>>> 91 }
>>>> 92 popThreadFrame(mt); // pop the frame.
>>>> 93 resumeThread(mt); // resume the thread.
>>>>    94             mt.join();
>>>>    95             // wait till the other thread completes its 
>>>> execution.
>>>>    96             System.out.println(" Thread state after poping / 
>>>> redefining = "+mt.threadState);
>>>>    97         } catch(Exception ie) {
>>>>    98             ie.printStackTrace();
>>>>    99         }
>>>>   100         if ( ( mt.threadState < 1000 ) &&
>>>>   101                 ( redefineAttempted() && isRedefined())  &&
>>>>   102                 agentStatus() ) {
>>>>   103             passed = true;
>>>>   104         }
>>>>   105         return passed;
>>>>   106     }
>>>>   It seems, the two checks ( redefineAttempted() && isRedefined()) 
>>>> at L101 are not needed any more.
>>>> Now, there is no way out of the while loop at the line 89.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>
>>>> On 6/19/18 11:42, Gary Adams wrote:
>>>>> There are some rare race conditions that impact some jvmti tests that
>>>>> suspend and resume threads. These tests were recently moved into
>>>>> the open repos.
>>>>>
>>>>>   Webrev: http://cr.openjdk.java.net/~gadams/6545967/webrev.00/
>>>>>
>>>>> The fix in hs203t003 replaces a blind 10 second sleep, with a 
>>>>> specific
>>>>> check to wait for a redefineClass to be performed, which takes 
>>>>> place as
>>>>> part of callbackFieldAccess. This let's the rest of the operations 
>>>>> be performed
>>>>> at a known point in the test sequence.
>>>>>
>>>>> The fix in sp05t003 moves the incrementing of the counters to 
>>>>> after the
>>>>> suspend thread calls. The agentProc function is watching the 
>>>>> counters to know
>>>>> when to continue processing and eventually resuming of the 
>>>>> suspended threads.
>>>>>
>>>>
>>>
>>
>
>