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:46:53 UTC 2018
I used the same declaration used for "instruction" variable,
which was already being used for getValue() calls.
I think we would want volatile if this was thread to thread
communication, but these variables are being used
for interprocess communication between the debugger and a
debuggee process. There are no reader/writer race conditions
here.
On 8/6/18, 1:36 PM, serguei.spitsyn at oracle.com wrote:
> Hi Gary,
>
> I was going to sponsor this but wanted to look at the latest webrev first.
> And found this:
> 44 static int testCase = -1;
>
> Should it be declared as volatile?
>
> Thanks,
> Serguei
>
>
> On 8/6/18 05:27, Gary Adams wrote:
>> I'll need a sponsor for the patch attached.
>>
>> On 8/6/18, 7: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.
>>> 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/20180806/b15c0760/attachment-0001.html>
More information about the serviceability-dev
mailing list