RFR JDK-8170089: nsk/jdi/EventSet/resume/resume008: ERROR: suspendCounts don't match for : Common-Cleaner
Gary Adams
gary.adams at oracle.com
Mon Aug 6 18:41:58 UTC 2018
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.
>
> 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