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

Gary Adams gary.adams at oracle.com
Fri Aug 24 12:32:46 UTC 2018


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