RFR JDK-8170089: nsk/jdi/EventSet/resume/resume008: ERROR: suspendCounts don't match for : Common-Cleaner
Gary Adams
gary.adams at oracle.com
Thu Aug 30 15:06:52 UTC 2018
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/20180830/ed86ddac/attachment-0001.html>
More information about the serviceability-dev
mailing list