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

Gary Adams gary.adams at oracle.com
Thu Aug 30 15:06:52 UTC 2018


Filed:

   https://bugs.openjdk.java.net/browse/JDK-8210224

for additional cleanup of the resume tests and better
use of the test library for shared code.

On 8/29/18, 4:16 PM, serguei.spitsyn at oracle.com wrote:
> On 8/29/18 07:46, Gary Adams wrote:
>> Since the vmTestbase/nsk tests are in need of
>> reformatting and refactoring, I've tried to isolate
>> changes to just the leaf test source files. The fix
>> has been duplicated in the 10 resume tests that
>> shared the same issue.
>>
>> I'd prefer to get this fix in as is and leave any test library
>> refactoring to a future issue.
>
> I'm Okay with that but could you, please, file a bug to sort it out 
> separately?
>
> Thanks,
> Serguei
>
>
>>
>> On 8/28/18, 5:23 PM, serguei.spitsyn at oracle.com wrote:
>>> 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());
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180830/ed86ddac/attachment-0001.html>


More information about the serviceability-dev mailing list