RFR 8214148: [TESTBUG] serviceability/tmtools/jstack/WaitNotifyThreadTest.java is not doing what is expected

[Daniel D. Daugherty]

Adding serviceability-***@... since the 'tmtools' including 'jstack'
are owned by the Serviceability team.

Dan

Hi all,
Could you review this fix for test
serviceability/tmtools/jstack/WaitNotifyThreadTest.java?
On one hand the test was not properly checking what it was intended to
check, since as mentioned in JBS the logic for checking the method
name was wrong. Also since there was only one monitor in the test, the
"for" loop with the message "waiting to re-lock in wait()" was never
actually reached.
Additionally, with change 8150689 the message "waiting to re-lock in
wait()" is now shown in the frame where the relocking is actually
taking place, so the logic for checking that should change.
I fixed the first issues and added logic to check for the "waiting to
re-lock in wait()" case. I used the Thread.State attribute to check
desire states are reached before getting the thread dump reports
through jstack. I run the test in mach5 several times for all
platforms and they all passed.
Webrev URL: http://cr.openjdk.java.net/~pchilanomate/8214148.01/webrev
Bug URL: https://bugs.openjdk.java.net/browse/JDK-8214148
<https://bugs.openjdk.java.net/browse/JDK-8150689>
Thanks,
Patricio

[David Holmes]

Hi Patricio,

This seems very complicated and I'm not quite seeing how it all goes
together. The check for waiting to re-lock now seems to dominant the
test and obscure the original checks.

I'm not sure this is worthwhile in the context of this test. It might be
much simpler to just get rid of the existing "waiting to re-lock" check
which will not be seen and then if we really want to check that case add
a much simpler form that just checks for that.

To me the simplest way to see the "re-lock in wait" case is to just:

synchronized(obj) {
obj.notifyAll();
<= take stack dump here =>
}

Cheers,
David

Post by Daniel D. Daugherty
are owned by the Serviceability team.
Dan

Hi all,
Could you review this fix for test
serviceability/tmtools/jstack/WaitNotifyThreadTest.java?
On one hand the test was not properly checking what it was intended to
check, since as mentioned in JBS the logic for checking the method
name was wrong. Also since there was only one monitor in the test, the
"for" loop with the message "waiting to re-lock in wait()" was never
actually reached.
Additionally, with change 8150689 the message "waiting to re-lock in
wait()" is now shown in the frame where the relocking is actually
taking place, so the logic for checking that should change.
I fixed the first issues and added logic to check for the "waiting to
re-lock in wait()" case. I used the Thread.State attribute to check
desire states are reached before getting the thread dump reports
through jstack. I run the test in mach5 several times for all
platforms and they all passed.
Webrev URL: http://cr.openjdk.java.net/~pchilanomate/8214148.01/webrev
Bug URL: https://bugs.openjdk.java.net/browse/JDK-8214148
<https://bugs.openjdk.java.net/browse/JDK-8150689>
Thanks,
Patricio

[Patricio Chilano]

Hi David,

Post by David Holmes
Hi Patricio,
This seems very complicated and I'm not quite seeing how it all goes
together. The check for waiting to re-lock now seems to dominant the
test and obscure the original checks.
I'm not sure this is worthwhile in the context of this test. It might
be much simpler to just get rid of the existing "waiting to re-lock"
check which will not be seen and then if we really want to check that
case add a much simpler form that just checks for that.

Ok, I actually had similar thoughts while I was adding the extra code.
Here is the new webrev:

http://cr.openjdk.java.net/~pchilanomate/8214148.02/webrev/

I removed the check for the "waiting to re-lock" case. I'm not sure if
it's okay to keep the change to
serviceability/tmtools/jstack/utils/DefaultFormat.java then. It doesn't
really affect this test, but it is needed for jstack to detect the locks
that appear in the stack report with the message "waiting to re-lock in
wait()".

Thanks,
Patricio

Post by David Holmes
synchronized(obj) {
   obj.notifyAll();
   <= take stack dump here =>
}
Cheers,
David

Post by Daniel D. Daugherty
are owned by the Serviceability team.
Dan

Hi all,
Could you review this fix for test
serviceability/tmtools/jstack/WaitNotifyThreadTest.java?
On one hand the test was not properly checking what it was intended
to check, since as mentioned in JBS the logic for checking the
method name was wrong. Also since there was only one monitor in the
test, the "for" loop with the message "waiting to re-lock in wait()"
was never actually reached.
Additionally, with change 8150689 the message "waiting to re-lock in
wait()" is now shown in the frame where the relocking is actually
taking place, so the logic for checking that should change.
I fixed the first issues and added logic to check for the "waiting
to re-lock in wait()" case. I used the Thread.State attribute to
check desire states are reached before getting the thread dump
reports through jstack. I run the test in mach5 several times for
all platforms and they all passed.
Webrev URL: http://cr.openjdk.java.net/~pchilanomate/8214148.01/webrev
Bug URL: https://bugs.openjdk.java.net/browse/JDK-8214148
<https://bugs.openjdk.java.net/browse/JDK-8150689>
Thanks,
Patricio

[David Holmes]

Hi Patricio,

Post by Patricio Chilano
Hi David,

Post by David Holmes
Hi Patricio,
This seems very complicated and I'm not quite seeing how it all goes
together. The check for waiting to re-lock now seems to dominant the
test and obscure the original checks.
I'm not sure this is worthwhile in the context of this test. It might
be much simpler to just get rid of the existing "waiting to re-lock"
check which will not be seen and then if we really want to check that
case add a much simpler form that just checks for that.

Ok, I actually had similar thoughts while I was adding the extra code.
http://cr.openjdk.java.net/~pchilanomate/8214148.02/webrev/
I removed the check for the "waiting to re-lock" case.

Good - the more I look at this test the more I see the "waiting to
re-lock" case is just not relevant to it.

The addition of the code to wait until the target thread is in the right
state seems good.

I'm unclear on some of the changes to the checking code in
analyzeThreadStackWaiting. It seems you removed the code that watched
for finding the wrong monitor and replaced that with a call to
assertMonitorInfo. But the latter is passed the address you got from the
MonitorInfo in the first place so the check of the address is never
going to fail. ?? That said you can't be waiting on two monitors so I
don't see how we can ever have the wrong one ??

A few minor style nits:

Pre-existing:

54 //Notify the waiting thread, so it stops waiting and sleeps

Please add a space after //

106 // Start athread that just waits

s/athread/a thread/

145 throw new RuntimeException(OBJECT_WAIT
146 + " method has to contain one lock
record but it contains " + mi.getLocks().size());

Indentation is wrong - the '+' should align with the O in OBJECT. Break
into three lines (at second + if needed)

New:

154 if(mi.getLocks().size() == 1){

Space after "if", and space before {

157 else{

Space before {

158 throw new RuntimeException(RUN_METHOD + " method
has to contain one lock record but it contains "
159 +
mi.getLocks().size());

Incorrect indentation - '+' should align with R in RUN

Post by Patricio Chilano
I'm not sure if
it's okay to keep the change to
serviceability/tmtools/jstack/utils/DefaultFormat.java then. It doesn't
really affect this test, but it is needed for jstack to detect the locks
that appear in the stack report with the message "waiting to re-lock in
wait()".

I'd probably revert that change at this stage.

Thanks,
David

Post by Patricio Chilano
Thanks,
Patricio

Post by David Holmes
synchronized(obj) {
   obj.notifyAll();
   <= take stack dump here =>
}
Cheers,
David

Post by Daniel D. Daugherty
are owned by the Serviceability team.
Dan

Hi all,
Could you review this fix for test
serviceability/tmtools/jstack/WaitNotifyThreadTest.java?
On one hand the test was not properly checking what it was intended
to check, since as mentioned in JBS the logic for checking the
method name was wrong. Also since there was only one monitor in the
test, the "for" loop with the message "waiting to re-lock in wait()"
was never actually reached.
Additionally, with change 8150689 the message "waiting to re-lock in
wait()" is now shown in the frame where the relocking is actually
taking place, so the logic for checking that should change.
I fixed the first issues and added logic to check for the "waiting
to re-lock in wait()" case. I used the Thread.State attribute to
check desire states are reached before getting the thread dump
reports through jstack. I run the test in mach5 several times for
all platforms and they all passed.
Webrev URL: http://cr.openjdk.java.net/~pchilanomate/8214148.01/webrev
Bug URL: https://bugs.openjdk.java.net/browse/JDK-8214148
<https://bugs.openjdk.java.net/browse/JDK-8150689>
Thanks,
Patricio

[Patricio Chilano]

Hi David,

Post by David Holmes
Hi Patricio,

Post by Patricio Chilano
Hi David,

Post by David Holmes
Hi Patricio,
This seems very complicated and I'm not quite seeing how it all goes
together. The check for waiting to re-lock now seems to dominant the
test and obscure the original checks.
I'm not sure this is worthwhile in the context of this test. It
might be much simpler to just get rid of the existing "waiting to
re-lock" check which will not be seen and then if we really want to
check that case add a much simpler form that just checks for that.

Ok, I actually had similar thoughts while I was adding the extra
http://cr.openjdk.java.net/~pchilanomate/8214148.02/webrev/
I removed the check for the "waiting to re-lock" case.

Good - the more I look at this test the more I see the "waiting to
re-lock" case is just not relevant to it.
The addition of the code to wait until the target thread is in the
right state seems good.
I'm unclear on some of the changes to the checking code in
analyzeThreadStackWaiting. It seems you removed the code that watched
for finding the wrong monitor and replaced that with a call to
assertMonitorInfo. But the latter is passed the address you got from
the MonitorInfo in the first place so the check of the address is
never going to fail. ??

I just replaced that "if-else" with assertMonitorInfo() because the same
check "monInfo.getType().equals("waiting on") &&
compareMonitorClass(monInfo)" will be done in assertMonitorInfo() and
the error case is also doing the same, so the code gets simplified. Yes,
the extra address check will never fail.

Post by David Holmes
That said you can't be waiting on two monitors so I don't see how we
can ever have the wrong one ??

I'm not sure why this test is checking for the monitor address in
assertMonitorInfo(). The only case where I see it could fail is for the
RUN_METHOD case if the address is null, but that has a separate check
before assertMonitorInfo(). Maybe at some point the test had more
monitors, because there was also that "for" loop checking for the
"waiting to re-lock in wait()" case. I can remove the test
"monInfo.getMonitorAddress().equals(monitorAddress)" in
assertMonitorInfo() but I don't think it hurts to keep it.

Post by David Holmes
54             //Notify the waiting thread, so it stops waiting and
sleeps
Please add a space after //
 106         // Start athread that just waits
s/athread/a thread/
 145                     throw new RuntimeException(OBJECT_WAIT
 146                             + " method has to contain one lock
record but it contains " + mi.getLocks().size());
Indentation is wrong - the '+' should align with the O in OBJECT.
Break into three lines (at second + if needed)
154                 if(mi.getLocks().size() == 1){
Space after "if", and space before {
 157                 else{
Space before {
158                     throw new RuntimeException(RUN_METHOD + "
method has to contain one lock record but it contains "
 159                                                             +
mi.getLocks().size());
Incorrect indentation - '+' should align with R in RUN

Done!

Post by David Holmes

Post by Patricio Chilano
I'm not sure if it's okay to keep the change to
serviceability/tmtools/jstack/utils/DefaultFormat.java then. It
doesn't really affect this test, but it is needed for jstack to
detect the locks that appear in the stack report with the message
"waiting to re-lock in wait()".

I'd probably revert that change at this stage.

Done!

Here is the new webrev:

http://cr.openjdk.java.net/~pchilanomate/8214148.03/webrev/


Thanks,
Patricio

Post by David Holmes
Thanks,
David

Post by Patricio Chilano
Thanks,
Patricio

Post by David Holmes
synchronized(obj) {
   obj.notifyAll();
   <= take stack dump here =>
}
Cheers,
David

Post by Daniel D. Daugherty
are owned by the Serviceability team.
Dan

Hi all,
Could you review this fix for test
serviceability/tmtools/jstack/WaitNotifyThreadTest.java?
On one hand the test was not properly checking what it was
intended to check, since as mentioned in JBS the logic for
checking the method name was wrong. Also since there was only one
monitor in the test, the "for" loop with the message "waiting to
re-lock in wait()" was never actually reached.
Additionally, with change 8150689 the message "waiting to re-lock
in wait()" is now shown in the frame where the relocking is
actually taking place, so the logic for checking that should change.
I fixed the first issues and added logic to check for the "waiting
to re-lock in wait()" case. I used the Thread.State attribute to
check desire states are reached before getting the thread dump
reports through jstack. I run the test in mach5 several times for
all platforms and they all passed.
http://cr.openjdk.java.net/~pchilanomate/8214148.01/webrev
Bug URL: https://bugs.openjdk.java.net/browse/JDK-8214148
<https://bugs.openjdk.java.net/browse/JDK-8150689>
Thanks,
Patricio

[David Holmes]

Looks good!

Thanks,
David

Post by Patricio Chilano
Hi David,

Post by David Holmes
Hi Patricio,

Post by Patricio Chilano
Hi David,

Post by David Holmes
Hi Patricio,
This seems very complicated and I'm not quite seeing how it all goes
together. The check for waiting to re-lock now seems to dominant the
test and obscure the original checks.
I'm not sure this is worthwhile in the context of this test. It
might be much simpler to just get rid of the existing "waiting to
re-lock" check which will not be seen and then if we really want to
check that case add a much simpler form that just checks for that.

Ok, I actually had similar thoughts while I was adding the extra
http://cr.openjdk.java.net/~pchilanomate/8214148.02/webrev/
I removed the check for the "waiting to re-lock" case.

Good - the more I look at this test the more I see the "waiting to
re-lock" case is just not relevant to it.
The addition of the code to wait until the target thread is in the
right state seems good.
I'm unclear on some of the changes to the checking code in
analyzeThreadStackWaiting. It seems you removed the code that watched
for finding the wrong monitor and replaced that with a call to
assertMonitorInfo. But the latter is passed the address you got from
the MonitorInfo in the first place so the check of the address is
never going to fail. ??

I just replaced that "if-else" with assertMonitorInfo() because the same
check "monInfo.getType().equals("waiting on") &&
compareMonitorClass(monInfo)" will be done in assertMonitorInfo() and
the error case is also doing the same, so the code gets simplified. Yes,
the extra address check will never fail.

Post by David Holmes
That said you can't be waiting on two monitors so I don't see how we
can ever have the wrong one ??

I'm not sure why this test is checking for the monitor address in
assertMonitorInfo(). The only case where I see it could fail is for the
RUN_METHOD case if the address is null, but that has a separate check
before assertMonitorInfo(). Maybe at some point the test had more
monitors, because there was also that "for" loop checking for the
"waiting to re-lock in wait()" case. I can remove the test
"monInfo.getMonitorAddress().equals(monitorAddress)" in
assertMonitorInfo() but I don't think it hurts to keep it.

Post by David Holmes
54             //Notify the waiting thread, so it stops waiting and
sleeps
Please add a space after //
 106         // Start athread that just waits
s/athread/a thread/
 145                     throw new RuntimeException(OBJECT_WAIT
 146                             + " method has to contain one lock
record but it contains " + mi.getLocks().size());
Indentation is wrong - the '+' should align with the O in OBJECT.
Break into three lines (at second + if needed)
154                 if(mi.getLocks().size() == 1){
Space after "if", and space before {
 157                 else{
Space before {
158                     throw new RuntimeException(RUN_METHOD + "
method has to contain one lock record but it contains "
 159                                                             +
mi.getLocks().size());
Incorrect indentation - '+' should align with R in RUN

Done!

Post by David Holmes

Post by Patricio Chilano
I'm not sure if it's okay to keep the change to
serviceability/tmtools/jstack/utils/DefaultFormat.java then. It
doesn't really affect this test, but it is needed for jstack to
detect the locks that appear in the stack report with the message
"waiting to re-lock in wait()".

I'd probably revert that change at this stage.

Done!
http://cr.openjdk.java.net/~pchilanomate/8214148.03/webrev/
Thanks,
Patricio

Post by David Holmes
Thanks,
David

Post by Patricio Chilano
Thanks,
Patricio

Post by David Holmes
synchronized(obj) {
   obj.notifyAll();
   <= take stack dump here =>
}
Cheers,
David

Post by Daniel D. Daugherty
are owned by the Serviceability team.
Dan

Hi all,
Could you review this fix for test
serviceability/tmtools/jstack/WaitNotifyThreadTest.java?
On one hand the test was not properly checking what it was
intended to check, since as mentioned in JBS the logic for
checking the method name was wrong. Also since there was only one
monitor in the test, the "for" loop with the message "waiting to
re-lock in wait()" was never actually reached.
Additionally, with change 8150689 the message "waiting to re-lock
in wait()" is now shown in the frame where the relocking is
actually taking place, so the logic for checking that should change.
I fixed the first issues and added logic to check for the "waiting
to re-lock in wait()" case. I used the Thread.State attribute to
check desire states are reached before getting the thread dump
reports through jstack. I run the test in mach5 several times for
all platforms and they all passed.
http://cr.openjdk.java.net/~pchilanomate/8214148.01/webrev
Bug URL: https://bugs.openjdk.java.net/browse/JDK-8214148
<https://bugs.openjdk.java.net/browse/JDK-8150689>
Thanks,
Patricio

[Patricio Chilano]

Thanks David!

Patricio

Post by David Holmes
Looks good!
Thanks,
David

Post by Patricio Chilano
Hi David,

Post by David Holmes
Hi Patricio,

Post by Patricio Chilano
Hi David,

Post by David Holmes
Hi Patricio,
This seems very complicated and I'm not quite seeing how it all
goes together. The check for waiting to re-lock now seems to
dominant the test and obscure the original checks.
I'm not sure this is worthwhile in the context of this test. It
might be much simpler to just get rid of the existing "waiting to
re-lock" check which will not be seen and then if we really want
to check that case add a much simpler form that just checks for that.

Ok, I actually had similar thoughts while I was adding the extra
http://cr.openjdk.java.net/~pchilanomate/8214148.02/webrev/
I removed the check for the "waiting to re-lock" case.

Good - the more I look at this test the more I see the "waiting to
re-lock" case is just not relevant to it.
The addition of the code to wait until the target thread is in the
right state seems good.
I'm unclear on some of the changes to the checking code in
analyzeThreadStackWaiting. It seems you removed the code that
watched for finding the wrong monitor and replaced that with a call
to assertMonitorInfo. But the latter is passed the address you got
from the MonitorInfo in the first place so the check of the address
is never going to fail. ??

I just replaced that "if-else" with assertMonitorInfo() because the
same check "monInfo.getType().equals("waiting on") &&
compareMonitorClass(monInfo)" will be done in assertMonitorInfo() and
the error case is also doing the same, so the code gets simplified.
Yes, the extra address check will never fail.

Post by David Holmes
That said you can't be waiting on two monitors so I don't see how we
can ever have the wrong one ??

I'm not sure why this test is checking for the monitor address in
assertMonitorInfo(). The only case where I see it could fail is for
the RUN_METHOD case if the address is null, but that has a separate
check before assertMonitorInfo(). Maybe at some point the test had
more monitors, because there was also that "for" loop checking for
the "waiting to re-lock in wait()" case. I can remove the test
"monInfo.getMonitorAddress().equals(monitorAddress)" in
assertMonitorInfo() but I don't think it hurts to keep it.

Post by David Holmes
54             //Notify the waiting thread, so it stops waiting and
sleeps
Please add a space after //
 106         // Start athread that just waits
s/athread/a thread/
 145                     throw new RuntimeException(OBJECT_WAIT
 146                             + " method has to contain one lock
record but it contains " + mi.getLocks().size());
Indentation is wrong - the '+' should align with the O in OBJECT.
Break into three lines (at second + if needed)
154                 if(mi.getLocks().size() == 1){
Space after "if", and space before {
 157                 else{
Space before {
158                     throw new RuntimeException(RUN_METHOD + "
method has to contain one lock record but it contains "
 159                                                             +
mi.getLocks().size());
Incorrect indentation - '+' should align with R in RUN

Done!

Post by David Holmes

Post by Patricio Chilano
I'm not sure if it's okay to keep the change to
serviceability/tmtools/jstack/utils/DefaultFormat.java then. It
doesn't really affect this test, but it is needed for jstack to
detect the locks that appear in the stack report with the message
"waiting to re-lock in wait()".

I'd probably revert that change at this stage.

Done!
http://cr.openjdk.java.net/~pchilanomate/8214148.03/webrev/
Thanks,
Patricio

Post by David Holmes
Thanks,
David

Post by Patricio Chilano
Thanks,
Patricio

Post by David Holmes
synchronized(obj) {
   obj.notifyAll();
   <= take stack dump here =>
}
Cheers,
David

Post by Daniel D. Daugherty
are owned by the Serviceability team.
Dan

Hi all,
Could you review this fix for test
serviceability/tmtools/jstack/WaitNotifyThreadTest.java?
On one hand the test was not properly checking what it was
intended to check, since as mentioned in JBS the logic for
checking the method name was wrong. Also since there was only
one monitor in the test, the "for" loop with the message
"waiting to re-lock in wait()" was never actually reached.
Additionally, with change 8150689 the message "waiting to
re-lock in wait()" is now shown in the frame where the relocking
is actually taking place, so the logic for checking that should
change.
I fixed the first issues and added logic to check for the
"waiting to re-lock in wait()" case. I used the Thread.State
attribute to check desire states are reached before getting the
thread dump reports through jstack. I run the test in mach5
several times for all platforms and they all passed.
http://cr.openjdk.java.net/~pchilanomate/8214148.01/webrev
Bug URL: https://bugs.openjdk.java.net/browse/JDK-8214148
<https://bugs.openjdk.java.net/browse/JDK-8150689>
Thanks,
Patricio