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