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:36:34 UTC 2018
Nevermind!
ThreadRefence/resume != Eventset/resume
On 8/31/18, 1:06 PM, Gary Adams wrote:
> 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/2fa3a5de/attachment-0001.html>
More information about the serviceability-dev
mailing list