[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR JDK-8170089: nsk/jdi/EventSet/resume/resume008: ERROR: suspendCounts don't match for : Commo
From: Gary Adams <gary.adams () oracle ! com>
Date: 2018-08-31 17:36:34
Message-ID: 5B897CA2.8080708 () oracle ! com
[Download RAW message or body]
Nevermind!
ThreadRefence/resume != Eventset/resume
On 8/31/18, 1:06 PM, Gary Adams wrote:
> https://bugs.openjdk.java.net/browse/JDK-8072701
> JDK-8072701: resume001 failed due to ERROR: timeout for waiting for a
> BreakpintEvent
>
> Should we close this as a duplicate?
>
> https://bugs.openjdk.java.net/browse/JDK-8201252
> JDK-8201252: unquarantine nsk/jdi/ThreadReference/resume/resume001
>
> Should we pull resume001 off the ProblemList.txt?
>
>
> On 8/30/18, 11:06 AM, Gary Adams wrote:
>> 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@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@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());
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Nevermind!<br>
ThreadRefence/resume != Eventset/resume<br>
<br>
On 8/31/18, 1:06 PM, Gary Adams wrote:
<blockquote cite="mid:5B8975A1.6070409@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8072701">https://bugs.openjdk.java.net/browse/JDK-8072701</a><br>
JDK-8072701: resume001 failed due to ERROR: timeout for waiting
for a BreakpintEvent<br>
<br>
Should we close this as a duplicate?<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8201252">https://bugs.openjdk.java.net/browse/JDK-8201252</a><br>
JDK-8201252: unquarantine nsk/jdi/ThreadReference/resume/resume001<br>
<br>
Should we pull resume001 off the ProblemList.txt?<br>
<br>
<br>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
On 8/30/18, 11:06 AM, Gary Adams wrote:
<blockquote cite="mid:5B88080C.7030902@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Filed: <br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8210224">https://bugs.openjdk.java.net/browse/JDK-8210224</a><br>
<br>
for additional cleanup of the resume tests and better<br>
use of the test library for shared code.<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<br>
On 8/29/18, 4:16 PM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:
<blockquote
cite="mid:75aae1ec-dd3f-d305-f6b7-81d1b720a22f@oracle.com"
type="cite">On 8/29/18 07:46, Gary Adams wrote: <br>
<blockquote type="cite">Since the vmTestbase/nsk tests are in
need of <br>
reformatting and refactoring, I've tried to isolate <br>
changes to just the leaf test source files. The fix <br>
has been duplicated in the 10 resume tests that <br>
shared the same issue. <br>
<br>
I'd prefer to get this fix in as is and leave any test
library <br>
refactoring to a future issue. <br>
</blockquote>
<br>
I'm Okay with that but could you, please, file a bug to sort
it out separately? <br>
<br>
Thanks, <br>
Serguei <br>
<br>
<br>
<blockquote type="cite"> <br>
On 8/28/18, 5:23 PM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote: <br>
<blockquote type="cite">Hi Gary, <br>
<br>
I'd suggest to put the informDebuggeeTestCase(int
testCase) <br>
and waitForTestCase(int t) into the test library so that
they <br>
are implemented just once. <br>
<br>
Thanks, <br>
Serguei <br>
<br>
<br>
On 8/28/18 05:20, Gary Adams wrote: <br>
<blockquote type="cite">I went back and confirmed that the
debuggeeClass initialization in <br>
TestDebuggerType1 RunThis() method happens very early on
in the <br>
test setup. If it was not initialized, the very first
attempts to set the <br>
breakpoint for communication would have failed. <br>
<br>
So this usage after a first test case is completed would
never be null. <br>
I've removed that check and attached a patch that should
be ready to <br>
push. <br>
<br>
On 8/27/18, 4:26 PM, Chris Plummer wrote: <br>
<blockquote type="cite">Hi Gary, <br>
<br>
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. <br>
<br>
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. <br>
<br>
Other than that the changes look fine. <br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
On 8/24/18 5:32 AM, Gary Adams wrote: <br>
<blockquote type="cite">Here's an updated webrev with
the isDisconnected checks removed <br>
in the setValue handling. <br>
<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="http://cr.openjdk.java.net/%7Egadams/8170089/webrev.02/index.html">http://cr.openjdk.java.net/~gadams/8170089/webrev.02/index.html</a>
<br>
<br>
Testing is in progress, but no failed tests have
shown up so far <br>
with the extra check removed. <br>
<br>
On 8/22/18, 1:05 PM, Gary Adams wrote: <br>
<blockquote type="cite">On 8/6/18, 3:16 PM, Chris
Plummer wrote: <br>
<blockquote type="cite">On 8/6/18 11:41 AM, Gary
Adams wrote: <br>
<blockquote type="cite">On 8/6/18, 1:56 PM,
Chris Plummer wrote: <br>
<blockquote type="cite">On 8/6/18 4:16 AM,
Gary Adams wrote: <br>
<blockquote type="cite">On 8/3/18, 6:38 PM,
Chris Plummer wrote: <br>
<blockquote type="cite">Hi Gary, <br>
<br>
Overall it looks good. <br>
<br>
Is the EventHandler.isDisconnected()
check needed? <br>
</blockquote>
This just follows the pattern used in
other calls to setValue. <br>
</blockquote>
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. <br>
</blockquote>
The best examples of checking
EventHandler.isDisconnected() <br>
can be seen in the implementation of
shouldRunAfterBreakPoint() <br>
See
\
test/hotspot/jtreg/vmTestbase/nsk/share/jdi/TestDebuggerType1.java <br>
<br>
It's used in the loop waiting for the
breakpoint event to be observed, <br>
and in the getValue() fetching of the next
"instruction" indicating <br>
testing is completed. <br>
</blockquote>
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? <br>
</blockquote>
<br>
Just following up on this loose end after vacation
... <br>
<br>
I agree that there is nothing preventing the
connection being terminated <br>
between the time isDisconnected() is checked and
the call to setValue() <br>
being made. I also don't see any harm in including
the isDisconnected() <br>
check here. If you think the test is improved by
removing the check, <br>
I'll make those changes, post a fresh webrev and
repeat the testing. <br>
<blockquote type="cite"> <br>
Chris <br>
<blockquote type="cite">
<blockquote type="cite"> <br>
Chris <br>
<blockquote type="cite">No point in
attempting the operation, if you know the
<br>
connection was lost. An exception at this
point could <br>
be misleading, if some other error has
already occurred. <br>
<blockquote type="cite"> <br>
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. <br>
</blockquote>
The original bug was reported against
resume008, so more time was spent in that
<br>
particular test, including some line
wrapping changes. I will restore the blank
lines <br>
you mentioned before producing a final
patch. The other tests had observed
failures <br>
also during testing. Applying the same
change fixed those failures as well. <br>
<blockquote type="cite"> <br>
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? <br>
</blockquote>
Reformatting or refactoring these older
tests would be at best a P5. <br>
I don't think it's worth filing a bug, but
as we fix bugs in these test it's <br>
worth some minimal amount of cleanup while
we are in the code. <br>
<blockquote type="cite"> <br>
thanks, <br>
<br>
Chris <br>
<br>
On 8/3/18 11:04 AM, Gary Adams wrote: <br>
<blockquote type="cite">Here is an
updated webrev with the alternate
solution implemented for <br>
resume 1 to 10. The debugger sets
testCase variable in the debuggee <br>
when each test case completes in the
debugger. By having the debuggee <br>
wait for the debugger to complete with
test case 0, it avoids the
interference <br>
that occurs by proceeding to the
breakpoint set in
MethodForCommunication <br>
before the debugger has compared
expected suspend counts. <br>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="http://cr.openjdk.java.net/%7Egadams/8170089/webrev.01/index.html">http://cr.openjdk.java.net/~gadams/8170089/webrev.01/index.html</a>
<br>
<br>
On 7/17/18, 11:33 AM, Gary Adams
wrote: <br>
<blockquote type="cite">A race
condition exists between the
debugger and the debuggee. <br>
<br>
The first test thread is started
with SUSPEND_NONE policy set. <br>
While processing the thread start
event the debugger captures <br>
an initial set of thread suspend
counts and resumes the <br>
debuggee vm. If the debuggee
advances quickly it reaches <br>
the breakpoint set for
methodForCommunication. Since the
breakpoint <br>
carries with it SUSPEND_ALL policy,
when the debugger captures a second
<br>
set of suspend counts, it will not
match the expected counts for <br>
a SUSPEND_NONE scenario. <br>
<br>
The proposed fix introduces a yield
in the debuggee test thread run
method <br>
to allow the debugger to get the
expected sampled values. <br>
<br>
Issue: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="https://bugs.openjdk.java.net/browse/JDK-8170089">https://bugs.openjdk.java.net/browse/JDK-8170089</a>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="http://cr.openjdk.java.net/%7Egadams/8170089/webrev.00/">http://cr.openjdk.java.net/~gadams/8170089/webrev.00/</a>
<br>
<br>
<br>
\
test/hotspot/jtreg/vmTestbase/nsk/share/jdi/TestDebuggerType1.java:
<br>
... <br>
186 private void
setCommunicationBreakpoint(ReferenceType
refType, String methodName) { <br>
187 Method method =
debuggee.methodByName(refType,
methodName); <br>
188 Location location
= null; <br>
189 try { <br>
190 location =
method.allLineLocations().get(0); <br>
191 } catch
(AbsentInformationException e) { <br>
192 throw new
Failure(e); <br>
193 } <br>
194 bpRequest =
debuggee.makeBreakpoint(location); <br>
195 <br>
<br>
196
\
bpRequest.setSuspendPolicy(EventRequest.SUSPEND_ALL); <br>
<br>
197
bpRequest.putProperty("number",
"zero"); <br>
198
bpRequest.enable(); <br>
199 <br>
200 eventHandler.addListener( <br>
201 new
EventHandler.EventListener() { <br>
202 \
public boolean eventReceived(Event event) {
<br>
203 \
if (event instanceof BreakpointEvent
&&
bpRequest.equals(event.request())) {
<br>
204 synchronized(eventHandler) {
<br>
205 display("Received
communication breakpoint event."); <br>
206 bpCount++; <br>
207 eventHandler.notifyAll(); <br>
208 \
} <br>
209 \
return true; <br>
210 \
} <br>
211 \
return false; <br>
212 \
} <br>
213 } <br>
214 ); <br>
215 } <br>
<br>
<br>
\
test/hotspot/jtreg/vmTestbase/nsk/jdi/EventSet/resume/resume008.java:
<br>
... <br>
140 display("......-->
vm.suspend();"); <br>
141
vm.suspend(); <br>
142 <br>
143 \
display(" getting : Map<String, Integer>
suspendsCounts1"); <br>
144 <br>
145
Map<String, Integer>
suspendsCounts1 = new
HashMap<String, Integer>(); <br>
146 \
for (ThreadReference threadReference :
vm.allThreads()) { <br>
147
suspendsCounts1.put(threadReference.name(),
threadReference.suspendCount()); <br>
148 } \
<br> 149
display(suspendsCounts1.toString());
<br>
150 <br>
151 \
display(" eventSet.resume;"); <br>
152 eventSet.resume(); <br>
153 <br>
154 \
display(" getting : Map<String, Integer>
suspendsCounts2"); <br>
<br>
This is where the breakpoint is
encountered before the second set of
suspend counts is acquired. <br>
<br>
155
Map<String, Integer>
suspendsCounts2 = new
HashMap<String, Integer>(); <br>
156 \
for (ThreadReference threadReference :
vm.allThreads()) { <br>
157
suspendsCounts2.put(threadReference.name(),
threadReference.suspendCount()); <br>
158 } \
<br> 159
display(suspendsCounts2.toString());
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic