RFR JDK-8170089: nsk/jdi/EventSet/resume/resume008: ERROR: suspendCounts don't match for : Common-Cleaner
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Aug 29 20:16:09 UTC 2018
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());
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list