Hi Sunny,
This looks good!
- I’d like to avoid bug ids in the names of tests, so can you change the
directory name to say “DaemonThread”?
- The test takes quite a long time to run (~1 minute). I wonder if we should
shorten that by either starting less processes (currently 50) or letting each
process run for a shorter period (currently ~1 sec).
- I think we can reduce the noise by removing the System.out.println() output
from the DummyAgent.
- During one of my test runs I ran into the following failure:
*** java.lang.instrument ASSERTION FAILED ***: "error == JVMTI_ERROR_NONE" at
Reentrancy.c line: 133
It looks like the patch isn’t complete. I added the patch below and have not
seen a failure after that. Can you incorporate that change as well (if you
agree with it)?
Thanks,
/Staffan
--- a/src/share/instrument/Reentrancy.c
+++ b/src/share/instrument/Reentrancy.c
@@ -130,6 +130,7 @@
error = confirmingTLSSet ( jvmtienv,
thread,
JPLIS_CURRENTLY_INSIDE_TOKEN);
+ check_phase_ret_false(error);
jplis_assert(error == JVMTI_ERROR_NONE);
if ( error != JVMTI_ERROR_NONE ) {
result = JNI_FALSE;
On 27 jan 2014, at 12:44, Chan, Sunny <sunny.c...@gs.com> wrote:
> Hi Staffan,
>
> I have attached the new version of the patch - I have reworked the test case
> and now it is mostly based in Java, but I have decided to keep using the
> shell script to build the Agent Jar file as it is easier.
>
> Thanks.
>
> From: Staffan Larsen [mailto:staffan.lar...@oracle.com]
> Sent: 13 January 2014 12:37
> To: Chan, Sunny [Tech]
> Cc: serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net
> Subject: Re: [PATCH] 7142035 assert in j.l.instrument agents during shutdown
> when daemon thread is running
>
>
> On 8 jan 2014, at 16:48, Chan, Sunny <sunny.c...@gs.com> wrote:
>
>
> In terms of the bug fix itself does it look fine?
>
> Yes, it does.
>
> Thanks,
> /Staffan
> <7142035rev1.patch>
