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
Tue Aug 28 21:23:29 UTC 2018
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