RFR JDK-8170089: nsk/jdi/EventSet/resume/resume008: ERROR: suspendCounts don't match for : Common-Cleaner

Gary Adams gary.adams at oracle.com
Fri Aug 31 17:06:41 UTC 2018


https://bugs.openjdk.java.net/browse/JDK-8072701
JDK-8072701: resume001 failed due to ERROR: timeout for waiting for a 
BreakpintEvent

   Should we close this as a duplicate?

https://bugs.openjdk.java.net/browse/JDK-8201252
JDK-8201252: unquarantine nsk/jdi/ThreadReference/resume/resume001

   Should we pull resume001 off the ProblemList.txt?


On 8/30/18, 11:06 AM, Gary Adams wrote:
> Filed:
>
> https://bugs.openjdk.java.net/browse/JDK-8210224
>
> for additional cleanup of the resume tests and better
> use of the test library for shared code.
>
> On 8/29/18, 4:16 PM, serguei.spitsyn at oracle.com wrote:
>> On 8/29/18 07:46, Gary Adams wrote:
>>> Since the vmTestbase/nsk tests are in need of
>>> reformatting and refactoring, I've tried to isolate
>>> changes to just the leaf test source files. The fix
>>> has been duplicated in the 10 resume tests that
>>> shared the same issue.
>>>
>>> I'd prefer to get this fix in as is and leave any test library
>>> refactoring to a future issue.
>>
>> I'm Okay with that but could you, please, file a bug to sort it out 
>> separately?
>>
>> Thanks,
>> Serguei
>>
>>
>>>
>>> On 8/28/18, 5:23 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Gary,
>>>>
>>>> I'd suggest to put the informDebuggeeTestCase(int testCase)
>>>> and waitForTestCase(int t) into the test library so that they
>>>> are implemented just once.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 8/28/18 05:20, Gary Adams wrote:
>>>>> I went back and confirmed that the debuggeeClass initialization in
>>>>> TestDebuggerType1 RunThis() method happens very early on in the
>>>>> test setup. If it was not initialized, the very first attempts to 
>>>>> set the
>>>>> breakpoint for communication would have failed.
>>>>>
>>>>> So this usage after a first test case is completed would never be 
>>>>> null.
>>>>> I've removed that check and attached a patch that should be ready to
>>>>> push.
>>>>>
>>>>> On 8/27/18, 4:26 PM, Chris Plummer wrote:
>>>>>> Hi Gary,
>>>>>>
>>>>>> Just getting caught up again. To answer your earlier question, 
>>>>>> yes, I think removing the isDisconnected() check is an 
>>>>>> improvement since it's use was at best inconsistent, and leads 
>>>>>> the reader to think that this is something that is expected to 
>>>>>> happen. If it does happen, the test will still fail in an 
>>>>>> appropriate way, and adding the check can actually hide the failure.
>>>>>>
>>>>>> And looking at this again, I'm now wondering about the 
>>>>>> debuggeeClass != null check. Is it possible for it to ever be 
>>>>>> null? That kind of seems along the lines of the isDisconnected() 
>>>>>> check.
>>>>>>
>>>>>> Other than that the changes look fine.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 8/24/18 5:32 AM, Gary Adams wrote:
>>>>>>> Here's an updated webrev with the isDisconnected checks removed
>>>>>>> in the setValue handling.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~gadams/8170089/webrev.02/index.html
>>>>>>>
>>>>>>> Testing is in progress, but no failed tests have shown up so far
>>>>>>> with the extra check removed.
>>>>>>>
>>>>>>> On 8/22/18, 1:05 PM, Gary Adams wrote:
>>>>>>>> On 8/6/18, 3:16 PM, Chris Plummer wrote:
>>>>>>>>> On 8/6/18 11:41 AM, Gary Adams wrote:
>>>>>>>>>> On 8/6/18, 1:56 PM, Chris Plummer wrote:
>>>>>>>>>>> On 8/6/18 4:16 AM, Gary Adams wrote:
>>>>>>>>>>>> On 8/3/18, 6:38 PM, Chris Plummer wrote:
>>>>>>>>>>>>> Hi Gary,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Overall it looks good.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Is the EventHandler.isDisconnected() check needed?
>>>>>>>>>>>> This just follows the pattern used in other calls to setValue.
>>>>>>>>>>> I'm not seeing any other examples of this. Can you point me 
>>>>>>>>>>> to them? Isn't it expected that you will always be 
>>>>>>>>>>> connected, and it will only be disconnected if there is 
>>>>>>>>>>> something very wrong with the execution of the test? Not 
>>>>>>>>>>> producing an error in that case could actually be 
>>>>>>>>>>> misleading, causing the test to fail with some sort of state 
>>>>>>>>>>> related error rather than some sort of exception indicating 
>>>>>>>>>>> it was disconnected.
>>>>>>>>>> The best examples of checking EventHandler.isDisconnected()
>>>>>>>>>> can be seen in the implementation of shouldRunAfterBreakPoint()
>>>>>>>>>> See 
>>>>>>>>>> test/hotspot/jtreg/vmTestbase/nsk/share/jdi/TestDebuggerType1.java 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It's used in the loop waiting for the breakpoint event to be 
>>>>>>>>>> observed,
>>>>>>>>>> and in the getValue() fetching of the next "instruction" 
>>>>>>>>>> indicating
>>>>>>>>>> testing is completed.
>>>>>>>>> Well, that's just 2 uses of isDisconnected() out of the 200+ 
>>>>>>>>> get/setValue() calls. I can see its use in the loop, since it 
>>>>>>>>> is used to force the exit of the loop when disconnected 
>>>>>>>>> (rather than waiting for timeout). The one before the 
>>>>>>>>> getValue() call is more like your use, and I don't see the 
>>>>>>>>> need in this case either. What's to prevent becoming 
>>>>>>>>> disconnected between the isDisconnected() and the 
>>>>>>>>> get/setValue() call?
>>>>>>>>
>>>>>>>> Just following up on this loose end after vacation ...
>>>>>>>>
>>>>>>>> I agree that there is nothing preventing the connection being 
>>>>>>>> terminated
>>>>>>>> between the time isDisconnected() is checked and the call to 
>>>>>>>> setValue()
>>>>>>>> being made. I also don't see any harm in including the 
>>>>>>>> isDisconnected()
>>>>>>>> check here. If you think the test is improved by removing the 
>>>>>>>> check,
>>>>>>>> I'll make those changes, post a fresh webrev and repeat the 
>>>>>>>> testing.
>>>>>>>>>
>>>>>>>>> Chris
>>>>>>>>>>>
>>>>>>>>>>> Chris
>>>>>>>>>>>> No point in attempting the operation, if you know the
>>>>>>>>>>>> connection was lost. An exception at this point could
>>>>>>>>>>>> be misleading, if some other error has already occurred.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In resume008a.java you removed a lot of empty lines. In 
>>>>>>>>>>>>> some places it's fine, but the lines at 132 and 134 should 
>>>>>>>>>>>>> have remained. Also, for the ones that were ok to remove, 
>>>>>>>>>>>>> I don't see you doing the same thing in the other files. I 
>>>>>>>>>>>>> think probably it's best to be consistent, and for this 
>>>>>>>>>>>>> webrev probably best not to do them since it distracts too 
>>>>>>>>>>>>> much from the important changes.
>>>>>>>>>>>> The original bug was reported against resume008, so more 
>>>>>>>>>>>> time was spent in that
>>>>>>>>>>>> particular test, including some line wrapping changes. I 
>>>>>>>>>>>> will restore the blank lines
>>>>>>>>>>>> you mentioned before producing a final patch. The other 
>>>>>>>>>>>> tests had observed failures
>>>>>>>>>>>> also during testing. Applying the same change fixed those 
>>>>>>>>>>>> failures as well.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Seems like there is a lot of abstraction that could have 
>>>>>>>>>>>>> been done with these tests to share a lot of code, but 
>>>>>>>>>>>>> since so far that hasn't been done, probably not a good 
>>>>>>>>>>>>> idea to start doing that with this fix. Do you think it's 
>>>>>>>>>>>>> worth filing an enhancement request for?
>>>>>>>>>>>> Reformatting or refactoring these older tests would be at 
>>>>>>>>>>>> best a P5.
>>>>>>>>>>>> I don't think it's worth filing a bug, but as we fix bugs 
>>>>>>>>>>>> in these test it's
>>>>>>>>>>>> worth some minimal amount of cleanup while we are in the code.
>>>>>>>>>>>>>
>>>>>>>>>>>>> thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Chris
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 8/3/18 11:04 AM, Gary Adams wrote:
>>>>>>>>>>>>>> Here is an updated webrev with the alternate solution 
>>>>>>>>>>>>>> implemented for
>>>>>>>>>>>>>> resume 1 to 10. The debugger sets testCase variable in 
>>>>>>>>>>>>>> the debuggee
>>>>>>>>>>>>>> when each test case completes in the debugger. By having 
>>>>>>>>>>>>>> the debuggee
>>>>>>>>>>>>>> wait for the debugger to complete with test case 0, it 
>>>>>>>>>>>>>> avoids the interference
>>>>>>>>>>>>>> that occurs by proceeding to the breakpoint set in 
>>>>>>>>>>>>>> MethodForCommunication
>>>>>>>>>>>>>> before the debugger has compared expected suspend counts.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>   Webrev: 
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~gadams/8170089/webrev.01/index.html 
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 7/17/18, 11:33 AM, Gary Adams wrote:
>>>>>>>>>>>>>>> A race condition exists between the debugger and the 
>>>>>>>>>>>>>>> debuggee.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The first test thread is started with SUSPEND_NONE 
>>>>>>>>>>>>>>> policy set.
>>>>>>>>>>>>>>> While processing the thread start event the debugger 
>>>>>>>>>>>>>>> captures
>>>>>>>>>>>>>>> an initial set of thread suspend counts and resumes the
>>>>>>>>>>>>>>> debuggee vm. If the debuggee advances quickly it reaches
>>>>>>>>>>>>>>> the breakpoint set for methodForCommunication. Since the 
>>>>>>>>>>>>>>> breakpoint
>>>>>>>>>>>>>>> carries with it SUSPEND_ALL policy, when the debugger 
>>>>>>>>>>>>>>> captures a second
>>>>>>>>>>>>>>> set of suspend counts, it will not match the expected 
>>>>>>>>>>>>>>> counts for
>>>>>>>>>>>>>>> a SUSPEND_NONE scenario.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The proposed fix introduces a yield in the debuggee test 
>>>>>>>>>>>>>>> thread run method
>>>>>>>>>>>>>>> to allow the debugger to get the expected sampled values.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8170089
>>>>>>>>>>>>>>>   Webrev: 
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~gadams/8170089/webrev.00/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> test/hotspot/jtreg/vmTestbase/nsk/share/jdi/TestDebuggerType1.java: 
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>>    186        private void 
>>>>>>>>>>>>>>> setCommunicationBreakpoint(ReferenceType refType, String 
>>>>>>>>>>>>>>> methodName) {
>>>>>>>>>>>>>>>    187            Method method = 
>>>>>>>>>>>>>>> debuggee.methodByName(refType, methodName);
>>>>>>>>>>>>>>>    188            Location location = null;
>>>>>>>>>>>>>>>    189            try {
>>>>>>>>>>>>>>>    190                location = 
>>>>>>>>>>>>>>> method.allLineLocations().get(0);
>>>>>>>>>>>>>>>    191            } catch (AbsentInformationException e) {
>>>>>>>>>>>>>>>    192                throw new Failure(e);
>>>>>>>>>>>>>>>    193            }
>>>>>>>>>>>>>>>    194            bpRequest = 
>>>>>>>>>>>>>>> debuggee.makeBreakpoint(location);
>>>>>>>>>>>>>>>    195
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>    196 
>>>>>>>>>>>>>>> bpRequest.setSuspendPolicy(EventRequest.SUSPEND_ALL);
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>    197 bpRequest.putProperty("number", "zero");
>>>>>>>>>>>>>>>    198            bpRequest.enable();
>>>>>>>>>>>>>>>    199
>>>>>>>>>>>>>>>    200 eventHandler.addListener(
>>>>>>>>>>>>>>>    201                 new EventHandler.EventListener() {
>>>>>>>>>>>>>>>    202                     public boolean 
>>>>>>>>>>>>>>> eventReceived(Event event) {
>>>>>>>>>>>>>>>    203                        if (event instanceof 
>>>>>>>>>>>>>>> BreakpointEvent && bpRequest.equals(event.request())) {
>>>>>>>>>>>>>>>    204 synchronized(eventHandler) {
>>>>>>>>>>>>>>>    205 display("Received communication breakpoint event.");
>>>>>>>>>>>>>>>    206 bpCount++;
>>>>>>>>>>>>>>>    207 eventHandler.notifyAll();
>>>>>>>>>>>>>>>    208                            }
>>>>>>>>>>>>>>>    209                            return true;
>>>>>>>>>>>>>>>    210                        }
>>>>>>>>>>>>>>>    211                        return false;
>>>>>>>>>>>>>>>    212                     }
>>>>>>>>>>>>>>>    213                 }
>>>>>>>>>>>>>>>    214            );
>>>>>>>>>>>>>>>    215        }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> test/hotspot/jtreg/vmTestbase/nsk/jdi/EventSet/resume/resume008.java: 
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>>    140 display("......--> vm.suspend();");
>>>>>>>>>>>>>>>    141                    vm.suspend();
>>>>>>>>>>>>>>>    142
>>>>>>>>>>>>>>>    143                    display(" getting : 
>>>>>>>>>>>>>>> Map<String, Integer> suspendsCounts1");
>>>>>>>>>>>>>>>    144
>>>>>>>>>>>>>>>    145                    Map<String, Integer> 
>>>>>>>>>>>>>>> suspendsCounts1 = new HashMap<String, Integer>();
>>>>>>>>>>>>>>>    146                    for (ThreadReference 
>>>>>>>>>>>>>>> threadReference : vm.allThreads()) {
>>>>>>>>>>>>>>>    147 suspendsCounts1.put(threadReference.name(), 
>>>>>>>>>>>>>>> threadReference.suspendCount());
>>>>>>>>>>>>>>>    148                    }
>>>>>>>>>>>>>>>    149 display(suspendsCounts1.toString());
>>>>>>>>>>>>>>>    150
>>>>>>>>>>>>>>>    151                    display(" eventSet.resume;");
>>>>>>>>>>>>>>>    152 eventSet.resume();
>>>>>>>>>>>>>>>    153
>>>>>>>>>>>>>>>    154                    display(" getting : 
>>>>>>>>>>>>>>> Map<String, Integer> suspendsCounts2");
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This is where the breakpoint is encountered before the 
>>>>>>>>>>>>>>> second set of suspend counts is acquired.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>    155                    Map<String, Integer> 
>>>>>>>>>>>>>>> suspendsCounts2 = new HashMap<String, Integer>();
>>>>>>>>>>>>>>>    156                    for (ThreadReference 
>>>>>>>>>>>>>>> threadReference : vm.allThreads()) {
>>>>>>>>>>>>>>>    157 suspendsCounts2.put(threadReference.name(), 
>>>>>>>>>>>>>>> threadReference.suspendCount());
>>>>>>>>>>>>>>>    158                    }
>>>>>>>>>>>>>>>    159 display(suspendsCounts2.toString());
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180831/10157ee9/attachment-0001.html>


More information about the serviceability-dev mailing list