RFR JDK-8170089: nsk/jdi/EventSet/resume/resume008: ERROR: suspendCounts don't match for : Common-Cleaner

Gary Adams gary.adams at oracle.com
Wed Aug 22 17:05:16 UTC 2018


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