[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
From: Mattias Tobiasson <mattias.tobiasson () oracle ! com>
Date: 2013-11-28 12:56:47
Message-ID: 4a6b7b07-eb55-48b1-9d0c-086a8fa2c3a5 () default
[Download RAW message or body]
The change has been updated after review.
Current summary of changes:
1. To avoid the complication of shared archives, the test now uses a dummy class for \
the retransform test. A new bug has been created for the possible problem of \
transforming classes loaded from archive: \
https://bugs.openjdk.java.net/browse/JDK-8029334
2. Removed all bash scripts.
3. Merged the setup bash script and the java test code for each test to a single java \
file. The old java test code is unchanged, it has just been moved to static nested \
class in the new java setup file.
4. Added some utility functions to jdk/testlibrary.
webrev:
http://cr.openjdk.java.net/~miauno/6461635/webrev.03/
bug:
https://bugs.openjdk.java.net/browse/JDK-6461635
Mattias
----- Original Message -----
From: leonid.mesnik@oracle.com
To: mattias.tobiasson@oracle.com
Cc: daniel.daugherty@oracle.com, serviceability-dev@openjdk.java.net, \
david.holmes@oracle.com
Sent: Thursday, November 28, 2013 11:02:30 AM GMT +01:00 Amsterdam / Berlin / Bern / \
Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
Mattias
On 11/28/2013 01:48 PM, Mattias Tobiasson wrote:
Is it ok if I check in the current test as it is now, and add a new bug for the \
(possible) problem of retransforming classes from the shared archive?
It is ok if you check current test which redefine non-shared class and add new bug \
about shared transformation. The usage of non-shared class is cleaner rather \
disabling sharing explicitly.
However you still need a review for this fix.
Reasons for why I want to check in the current test as it is are:
The purpose of the current test is not about the shared archive and is already quite \
large. A simpler reproducer could be used for the new bug. I think it would be \
better to add more testcases into this test or more tests with similar but different \
scenarios. The new tests could be part of fix of new issue which you are going to \
file.
Leonid
The new bug may not even be a bug, but working as expected. I want someone else to \
look at the bug before adding a new test. This fix also contains updates for the \
testlibrary that are needed for other tests. This bug was reported in 2006, so it is \
not a regression in jdk8.
Mattias
----- Original Message -----
From: leonid.mesnik@oracle.com
To: mattias.tobiasson@oracle.com , david.holmes@oracle.com
Cc: serviceability-dev@openjdk.java.net , daniel.daugherty@oracle.com
Sent: Wednesday, November 27, 2013 4:15:09 PM GMT +01:00 Amsterdam / Berlin / Bern / \
Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
I think test should test instrumentation of custom, JDK, JDK shared classes as a part \
of scenario. This is how tools could use this mechanism. May be I am wrong.
Also I found
• JDK-5002268 Allow class sharing use with RedefineClasses
which allow to redefine of classes when class sharing is used. I don't exactly know \
should it work for j.l.instrument as well as for JVMTI
Dan
Could you please help us with this? Is it a legal scenario to redefine shared \
classes?
Leonid
On 11/27/2013 03:46 PM, Mattias Tobiasson wrote:
According to the test documentation and bug references the test verifies the manifest \
attribute "Can-Redefine-Classes". The test does not mention shared class archive or \
-Xshare. But maybe the test has found a problem accidentally...
Mattias
----- Original Message -----
From: david.holmes@oracle.com To: mattias.tobiasson@oracle.com Cc: \
serviceability-dev@openjdk.java.net Sent: Wednesday, November 27, 2013 12:21:50 PM \
GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
On 27/11/2013 8:46 PM, Mattias Tobiasson wrote:
Hi, I now have a reproducer for this test that fails every time.
I just need to verify that it really is a test bug and not a product bug.
This is a summary of the test:
1. It loads a javaagent jar that implements ClassFileTransformer.
2. The agent loads class java.math.BigInteger and expects a callback to \
ClassFileTransformer.transform(). But it sometimes does not get that callback.
I believe the reason for the intermittent failures are that the jvm flag -Xshare have \
different default values on different servers. -Xshare:auto will default to off for \
server compiler, but on for client
When the reproducer is run with flag "-Xshare:off" it passes every time.
When run with "-Xshare:auto" it fails every time.
When the reproducer is changed to use a dummy class (RedefineDummy) instead of \
BigInteger, then the test passes every time no matter what value -Xshare has.
My proposed fix for the bug is to use a dummy class instead of BigInteger. I just \
need to verify that it is ok that ClassFileTransformer.transform() is NOT called for \
BigInteger when -Xshare is enabled. If the whole point of the test is to transform a \
file from the shared archive then changing to a class not in the archive will defeat \
that. So you need to verify what the intent of the test is.
David
The reproducer is shown below. It loads the javaagent into its own vm so I don't need \
to set up multiple jvms.
public class TestRedefine implements ClassFileTransformer {
public static void agentmain(String agentArgs, Instrumentation inst) throws \
Throwable { try {
inst.addTransformer(new TestRedefine());
ClassLoader.getSystemClassLoader().loadClass("java.math.BigInteger");
if (!isTransformCalled) {
throw new Exception("transform() NOT called for " + targetName);
}
} catch (Throwable t) {
t.printStackTrace();
throw t;
}
}
public byte[] transform(ClassLoader loader,
String className,
Class<?> classBeingRedefined,
ProtectionDomain protectionDomain,
byte[] classfileBuffer) {
System.out.println("transform called: " + className);
if (className.equals("java/math/BigInteger")) {
isTransformCalled = true;
}
return null;
}
public static void main(String args[]) throws Exception {
String pid = getProcessId();
try {
VirtualMachine vm = VirtualMachine.attach(pid);
vm.loadAgent("TestRedefine.jar");
} catch (Exception e) {
e.printStackTrace();
throw e;
}
}
... more code ...
}
----- Original Message -----
From: leonid.mesnik@oracle.com To: mattias.tobiasson@oracle.com Cc: \
serviceability-dev@openjdk.java.net Sent: Wednesday, November 27, 2013 9:19:37 AM GMT \
+01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
Hi
Generally I am ok with your comments and fix. However you still needed
to get official review.
The only question is about failure related from retransformation of
j.l.BigInteger with CDS.
Could it be JDK issue? In this case it would be better to file it as a
part of fix.
I understand that using of CDS and instrumentation of JDK classes is not
related with this test
and your fix make it cleaner. However could you please verify that we
don't have JDK issue here.
see other comments inline
On 11/26/2013 08:35 PM, Mattias Tobiasson wrote:
Hi,
Thanks for the review. I have summarized the questions and answers:
Q: What is the reason of this intermittent failure?
A: The test expects the callback ClassFileTransformer.transform() to be called when \
it loads a new class. That function does not seem to be called when class sharing is \
enabled and a previous test has called "-Xshare:dump". I am not really sure how \
ClassFileTransformer works together with class sharing. I got the idea for \
-Xshare:dump from this bug: https://bugs.openjdk.java.net/browse/JDK-6571866 I have \
verified on jprt that it fails without that flag. I have not been able to reproduce a \
failure when -Xshare:off is set.
Having thought more about the problem, I would like to use another fix. The test \
currently uses java.lang.BigInteger for the transform test. I want to change that and \
instead use my own class (RedefineDummy.java). Since loading my own class is not \
affected by -Xshare, I do not need to set the flag.
Q: You used "output" in finally which is not initialized properly in the case of \
Exception. Could we get another uncaught exception in finally?
A: output is only sent to getProcessLog() to get a log message. That function can \
handle null values, which it just logs as "null". Thank you for explanation.
Q: In getCommandLine(), should we also trim cmd to remove last " "?
A: Yes. I will fix that. I will also add a check if ProcessBuilder is null. Ok
Q: waitForJvmPid(String key) throws Throwable. Why throw throwable?
A: I think there are so many things that can go wrong when starting a separate java \
process, that I do not want to check for expected errors. I also don't know how a \
test writer should handle different kind of exceptions from this function. Any \
exception is logged in the sub function.
Q: Should tryFindJvmPid(String key) be private? Are we going to use it in tests or \
only waitForJvmPid?
A: Yes, that function may also be used by tests. It is currently not used by any \
test, but it might be useful. Ok
Q: Timeouts in function waitForJvmPid(String key)?
A: I definitely agree of the problem. The reason for not adding a timeout parameter \
to the function is that I do not want the tests to be responsible for setting hard \
coded timeouts. Timeouts should be handled by the framework. I know there are plans \
of adding better process handling to jtreg (with ProcessHandler). After that jtreg \
should be better at handling timeouts. I will add a flush() to stdout to make sure we \
log before we wait. Hope we will update jtreg before get into this issue :)
Leonid
Mattias
----- Original Message -----
From: leonid.mesnik@oracle.com To: mattias.tobiasson@oracle.com , \
serviceability-dev@openjdk.java.net Sent: Monday, November 25, 2013 6:26:27 PM GMT \
+01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
Hi
I have a couple of high-level questions.
On 11/25/2013 08:28 PM, Mattias Tobiasson wrote:
Hi,
The test has been updated after the first review.
The two java files for each test has been merged to a single file.
Updated summary of changes:
1. The real test bug fix is to add flag "-Xshare:off" when starting the "Application" \
instance. Without that flag, the test for ClassFileTransformer in RedefineAgent.java \
fails intermittently. The flag is added in function startApplication() in \
RunnerUtil.java. What is the reason of this intermittent failure?
2. Removed all bash scripts. Great!
3. Merged the setup bash script and the java test code for each test to a single java \
file. The old java test code is unchanged, it has just been moved to static nested \
class in the new java setup file.
4. Added some utility functions to jdk/testlibrary. Here some comments about library \
code. http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessThread.java.udiff.html \
+ try { + output = new OutputAnalyzer(this.process);
+ } catch (Throwable t) {
+ String name = Thread.currentThread().getName();
+ System.out.println(String.format("ProcessThread[%s] failed: %s", \
name, t.toString())); + throw t;
+ } finally {
+ String logMsg = ProcessTools.getProcessLog(processBuilder, output);
+ System.out.println(logMsg);
+ }
You used output in finally which is not initialized properly in the case
of Exception.
Could we get another uncaught exception in finally? \
http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.udiff.html \
1) Same in as previous in executeProcess method.
2) getCommandLine (...)
+ /**
+ * @return The full command line for the ProcessBuilder.
+ */
+ public static String getCommandLine(ProcessBuilder pb) {
+ StringBuilder cmd = new StringBuilder();
+ for (String s : pb.command()) {
+ cmd.append(s).append(" ");
+ }
+ return cmd.toString();
+ }
Should we also trim cmd to remove last " "? \
http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/Utils.java.udiff.html \
public static int waitForJvmPid(String key) throws Throwable {
Why it throw Trowable? Could we deal with exceptions in this method and
re-throw some more meaningful exception here?
Could you force flush for out here:
System.out.println("waitForJvmPid: Waiting for key '" + key + "'");
Hope it should be enough. It is scary to investigate such "timeouts".
Would it be better to add some
internal timeout in testlibrary? Really jtreg doesn't kill all processes
and we have alive java processes in such case.
For samevm tests timeouts are even worse. There is no good way to kill
test in samevm.
Should be
public static int tryFindJvmPid(String key) throws Throwable {
private? Are we going to use it in tests or only waitForJvmPid?
Leonid
5. Moved exit code check from the common utility function in ProcessThread.java to \
the test JstatdTest.java. The check is moved to the test because other tests in the \
future may have other expected exit codes.
Webrev: http://cr.openjdk.java.net/~miauno/6461635/webrev.01/ Bug: \
https://bugs.openjdk.java.net/browse/JDK-6461635 Mattias
----- Original Message -----
From: mattias.tobiasson@oracle.com To: mikael.auno@oracle.com Cc: \
serviceability-dev@openjdk.java.net , Alan.Bateman@oracle.com Sent: Thursday, \
November 21, 2013 9:22:11 AM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm \
/ Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
I agree that merging both files in a single file may be good.
The main reason why I did not do that is that I wanted to keep the history of the \
existing test. If I copy the test to a new file, all history of the code is lost.
But this test bug was reported 8 years ago, and I don't believe much has changed in \
the code since then. So keeping the history may not be that important.
Mattias
----- Original Message -----
From: mikael.auno@oracle.com To: mattias.tobiasson@oracle.com , \
Alan.Bateman@oracle.com Cc: serviceability-dev@openjdk.java.net Sent: Wednesday, \
November 20, 2013 4:11:45 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm \
/ Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
How about defining the class that you want to attach to as a static
inner class to the actual test? That would give you only one file, but
with two classes, each with its own main method and clear correlation
between them.
Mikael
On 2013-11-20 15:41, Mattias Tobiasson wrote:
Hi,
Each test requires 2 files, the actual test code and a helper file.
The helper file will launch a separate java process (called Application), and then \
start the actual test. The actual test will then attach to the Application.
For example:
PermissionTests.sh: Helper file that will launch Application instance and then start \
PermissionTest.java
PermissionTest.java: The actual test code that attaches to the Application.
It is the PermissionTests.sh that is started by jtreg (contains the "@test"-tag).
When I port PermissionTest.sh to java I would get 2 files called PermissionTest.java, \
so some name change is required.
I could have kept the old PermissionTest.java unchanged, but then I would need \
another name for the prevoius PermissionTest.sh. And I wanted a "clean" Test name for \
the file containing the "@test"-tag.
I used these names:
TestPermission.java: Helper file.
TestPermissionImpl.java: Actual test code.
I am still new to adding tests for open jdk. I am happy to change the names if they \
do not follow the naming convention.
Mattias
----- Original Message -----
From: Alan.Bateman@oracle.com To: mattias.tobiasson@oracle.com Cc: \
serviceability-dev@openjdk.java.net Sent: Wednesday, November 20, 2013 2:50:52 PM GMT \
+01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
Out of curiosity, what is the reason for the rename? I ask because we
use Basic and similar names in many areas. Also anything in the test
tree should be a test.
-Alan.
On 20/11/2013 13:47, Mattias Tobiasson wrote:
Hi,
Could you please review this fix.
Summary of changes:
1. The real test bug fix is to add flag "-Xshare:off" when starting the "Application" \
instance. Without that flag, the test for ClassFileTransformer in RedefineAgent.java \
fails intermittently. The flag is added in function startApplication() in \
RunnerUtil.java.
2. Ported the following bash scripts to java:
BasicTests.sh -> TestBasic.java
PermissionTests.sh -> TestPermission.java
ProviderTests.sh -> TestProvider.java
3. Renamed the java test code to avoid name clash with new java classes ported from \
bash script: BasicsTest.java -> TestBasicImpl.java
PermissionTest.java -> TestPermissionImpl.java
ProviderTest.java -> TestProviderImpl.java
4. Added some utility functions to jdk/testlibrary.
5. Moved exit code check from the common utility function in ProcessThread.java to \
the test JstatdTest.java. The check is moved to the test because other tests in the \
future may have other expected exit codes.
Thanks,
Mattias
Webrev: http://cr.openjdk.java.net/~miauno/6461635/webrev.00/ Bug: \
https://bugs.openjdk.java.net/browse/JDK-6461635
--
Leonid Mesnik
JVM SQE
--
Leonid Mesnik
JVM SQE
[Attachment #3 (text/html)]
<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div \
style='font-family: Times New Roman; font-size: 12pt; color: #000000'>The change has \
been updated after review.<br><br>Current summary of changes:<br>1. To avoid the \
complication of shared archives, the test now uses a dummy class for the retransform \
test. A new bug has been created for the possible problem of transforming classes \
loaded from archive: https://bugs.openjdk.java.net/browse/JDK-8029334<br><br>2. \
Removed all bash scripts.<br><br>3. Merged the setup bash script and the java test \
code for each test to a single java file. The old java test code is unchanged, it has \
just been moved to static nested class in the new java setup file.<br><br>4. Added \
some utility functions to \
jdk/testlibrary.<br><br>webrev:<br>http://cr.openjdk.java.net/~miauno/6461635/webrev.03/<br><br>bug:<br><a \
class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-6461635" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-6461635</a><br><br>Mattias<br><br><br>----- \
Original Message -----<br>From: leonid.mesnik@oracle.com<br>To: \
mattias.tobiasson@oracle.com<br>Cc: daniel.daugherty@oracle.com, \
serviceability-dev@openjdk.java.net, david.holmes@oracle.com<br>Sent: Thursday, \
November 28, 2013 11:02:30 AM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm \
/ Vienna<br>Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails \
intermittently<br><br>
<div>
<div class="moz-cite-prefix">Mattias<br>
<br>
On 11/28/2013 01:48 PM, Mattias Tobiasson wrote:<br>
</div>
<blockquote cite="mid:4aaddb4a-5eb3-48e9-924d-386ce929ac29@default">
<style>p { margin: 0; }</style>
<div style="font-family: Times New Roman; font-size: 12pt; color:
#000000">Is it ok if I check in the current test as it is now,
and add a new bug for the (possible) problem of retransforming
classes from the shared archive?<br>
<br>
</div>
</blockquote>
It is ok if you check current test which redefine non-shared class
and add new bug about shared transformation.<br>
The usage of non-shared class is cleaner rather disabling sharing
explicitly. <br>
<br>
However you still need a review for this fix. <br>
<blockquote cite="mid:4aaddb4a-5eb3-48e9-924d-386ce929ac29@default">
<div style="font-family: Times New Roman; font-size: 12pt; color:
#000000">Reasons for why I want to check in the current test as
it is are:<br>
The purpose of the current test is not about the shared archive
and is already quite large. A simpler reproducer could be used
for the new bug.<br>
</div>
</blockquote>
I think it would be better to add more testcases into this test or
more tests with similar but different scenarios.<br>
The new tests could be part of fix of new issue which you are going
to file. <br>
<br>
Leonid<br>
<blockquote cite="mid:4aaddb4a-5eb3-48e9-924d-386ce929ac29@default">
<div style="font-family: Times New Roman; font-size: 12pt; color:
#000000">The new bug may not even be a bug, but working as
expected. I want someone else to look at the bug before adding a
new test.<br>
This fix also contains updates for the testlibrary that are
needed for other tests.<br>
This bug was reported in 2006, so it is not a regression in
jdk8.<br>
<br>
Mattias<br>
<br>
----- Original Message -----<br>
From: <a class="moz-txt-link-abbreviated" \
href="mailto:leonid.mesnik@oracle.com" \
target="_blank">leonid.mesnik@oracle.com</a><br> To: <a \
class="moz-txt-link-abbreviated" href="mailto:mattias.tobiasson@oracle.com" \
target="_blank">mattias.tobiasson@oracle.com</a>, <a class="moz-txt-link-abbreviated" \
href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a><br>
Cc: <a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net" \
target="_blank">serviceability-dev@openjdk.java.net</a>,
<a class="moz-txt-link-abbreviated" href="mailto:daniel.daugherty@oracle.com" \
target="_blank">daniel.daugherty@oracle.com</a><br> Sent: Wednesday, November 27, \
2013 4:15:09 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna<br>
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test
fails intermittently<br>
<br>
<div>
<div class="moz-cite-prefix">I think test should test
instrumentation of custom, JDK, JDK shared classes as a part
of scenario.<br>
This is how tools could use this mechanism. May be I am
wrong.<br>
<br>
Also I found <br>
<ul class="breadcrumbs">
<li><a id="key-val" rel="4303491" \
href="https://bugs.openjdk.java.net/browse/JDK-5002268" \
target="_blank">JDK-5002268</a> Allow class sharing use with RedefineClasses </li>
</ul>
<p>which allow to redefine of classes when class sharing is
used. I don't exactly know should it work <br>
for j.l.instrument as well as for JVMTI <br>
</p>
<p>Dan <br>
Could you please help us with this? Is it a legal scenario
to redefine shared classes?<br>
</p>
<p><br>
Leonid<br>
</p>
<br>
On 11/27/2013 03:46 PM, Mattias Tobiasson wrote:<br>
</div>
<blockquote cite="mid:68c32d89-9858-4442-b7c7-1f00b1b9390f@default">
<pre>According to the test documentation and bug references the test \
verifies the manifest attribute "Can-Redefine-Classes". The test does not mention \
shared class archive or -Xshare. But maybe the test has found a problem \
accidentally...
Mattias
----- Original Message -----
From: <a class="moz-txt-link-abbreviated" href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a>
To: <a class="moz-txt-link-abbreviated" href="mailto:mattias.tobiasson@oracle.com" \
target="_blank">mattias.tobiasson@oracle.com</a>
Cc: <a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net" \
target="_blank">serviceability-dev@openjdk.java.net</a>
Sent: Wednesday, November 27, 2013 12:21:50 PM GMT +01:00 Amsterdam / Berlin / Bern / \
Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
On 27/11/2013 8:46 PM, Mattias Tobiasson wrote:
</pre>
<blockquote>
<pre>Hi, I now have a reproducer for this test that fails every time.
I just need to verify that it really is a test bug and not a product bug.
This is a summary of the test:
1. It loads a javaagent jar that implements ClassFileTransformer.
2. The agent loads class java.math.BigInteger and expects a callback to \
ClassFileTransformer.transform(). But it sometimes does not get that callback.
I believe the reason for the intermittent failures are that the jvm flag -Xshare have \
different default values on different servers. </pre>
</blockquote>
<pre>-Xshare:auto will default to off for server compiler, but on for \
client
</pre>
<blockquote>
<pre>When the reproducer is run with flag "-Xshare:off" it passes every \
time. When run with "-Xshare:auto" it fails every time.
When the reproducer is changed to use a dummy class (RedefineDummy) instead of \
BigInteger, then the test passes every time no matter what value -Xshare has.
My proposed fix for the bug is to use a dummy class instead of BigInteger. I just \
need to verify that it is ok that ClassFileTransformer.transform() is NOT called for \
BigInteger when -Xshare is enabled. </pre>
</blockquote>
<pre>If the whole point of the test is to transform a file from the \
shared archive then changing to a class not in the archive will defeat that. So
you need to verify what the intent of the test is.
David
</pre>
<blockquote>
<pre>The reproducer is shown below. It loads the javaagent into its own \
vm so I don't need to set up multiple jvms.
public class TestRedefine implements ClassFileTransformer {
public static void agentmain(String agentArgs, Instrumentation inst) throws \
Throwable { try {
inst.addTransformer(new TestRedefine());
ClassLoader.getSystemClassLoader().loadClass("java.math.BigInteger");
if (!isTransformCalled) {
throw new Exception("transform() NOT called for " + targetName);
}
} catch (Throwable t) {
t.printStackTrace();
throw t;
}
}
public byte[] transform(ClassLoader loader,
String className,
Class<?> classBeingRedefined,
ProtectionDomain protectionDomain,
byte[] classfileBuffer) {
System.out.println("transform called: " + className);
if (className.equals("java/math/BigInteger")) {
isTransformCalled = true;
}
return null;
}
public static void main(String args[]) throws Exception {
String pid = getProcessId();
try {
VirtualMachine vm = VirtualMachine.attach(pid);
vm.loadAgent("TestRedefine.jar");
} catch (Exception e) {
e.printStackTrace();
throw e;
}
}
... more code ...
}
----- Original Message -----
From: <a class="moz-txt-link-abbreviated" href="mailto:leonid.mesnik@oracle.com" \
target="_blank">leonid.mesnik@oracle.com</a>
To: <a class="moz-txt-link-abbreviated" href="mailto:mattias.tobiasson@oracle.com" \
target="_blank">mattias.tobiasson@oracle.com</a>
Cc: <a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net" \
target="_blank">serviceability-dev@openjdk.java.net</a>
Sent: Wednesday, November 27, 2013 9:19:37 AM GMT +01:00 Amsterdam / Berlin / Bern / \
Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
Hi
Generally I am ok with your comments and fix. However you still needed
to get official review.
The only question is about failure related from retransformation of
j.l.BigInteger with CDS.
Could it be JDK issue? In this case it would be better to file it as a
part of fix.
I understand that using of CDS and instrumentation of JDK classes is not
related with this test
and your fix make it cleaner. However could you please verify that we
don't have JDK issue here.
see other comments inline
On 11/26/2013 08:35 PM, Mattias Tobiasson wrote:
</pre>
<blockquote>
<pre>Hi,
Thanks for the review. I have summarized the questions and answers:
Q: What is the reason of this intermittent failure?
A: The test expects the callback ClassFileTransformer.transform() to be called when \
it loads a new class. That function does not seem to be called when class sharing is \
enabled and a previous test has called "-Xshare:dump". I am not really sure how \
ClassFileTransformer works together with class sharing. I got the idea for \
-Xshare:dump from this bug: <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-6571866" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-6571866</a> I have verified \
on jprt that it fails without that flag. I have not been able to reproduce a failure \
when -Xshare:off is set.
Having thought more about the problem, I would like to use another fix. The test \
currently uses java.lang.BigInteger for the transform test. I want to change that and \
instead use my own class (RedefineDummy.java). Since loading my own class is not \
affected by -Xshare, I do not need to set the flag. </pre>
</blockquote>
<blockquote>
<pre>Q: You used "output" in finally which is not initialized \
properly in the case of Exception. Could we get another uncaught exception in \
finally?
A: output is only sent to getProcessLog() to get a log message. That function can \
handle null values, which it just logs as "null". </pre>
</blockquote>
<pre>Thank you for explanation.
</pre>
<blockquote>
<pre>Q: In getCommandLine(), should we also trim cmd to remove last " \
"?
A: Yes. I will fix that. I will also add a check if ProcessBuilder is null.
</pre>
</blockquote>
<pre>Ok
</pre>
<blockquote>
<pre>Q: waitForJvmPid(String key) throws Throwable. Why throw \
throwable?
A: I think there are so many things that can go wrong when starting a separate java \
process, that I do not want to check for expected errors. I also don't know how a \
test writer should handle different kind of exceptions from this function. Any \
exception is logged in the sub function. </pre>
</blockquote>
<blockquote>
<pre>Q: Should tryFindJvmPid(String key) be private? Are we going to \
use it in tests or only waitForJvmPid?
A: Yes, that function may also be used by tests. It is currently not used by any \
test, but it might be useful. </pre>
</blockquote>
<pre>Ok
</pre>
<blockquote>
<pre>Q: Timeouts in function waitForJvmPid(String key)?
A: I definitely agree of the problem. The reason for not adding a timeout parameter \
to the function is that I do not want the tests to be responsible for setting hard \
coded timeouts. Timeouts should be handled by the framework. I know there are plans \
of adding better process handling to jtreg (with ProcessHandler). After that jtreg \
should be better at handling timeouts. I will add a flush() to stdout to make sure we \
log before we wait. </pre>
</blockquote>
<pre>Hope we will update jtreg before get into this issue :)
Leonid
</pre>
<blockquote>
<pre>Mattias
----- Original Message -----
From: <a class="moz-txt-link-abbreviated" href="mailto:leonid.mesnik@oracle.com" \
target="_blank">leonid.mesnik@oracle.com</a>
To: <a class="moz-txt-link-abbreviated" href="mailto:mattias.tobiasson@oracle.com" \
target="_blank">mattias.tobiasson@oracle.com</a>, <a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net" \
target="_blank">serviceability-dev@openjdk.java.net</a>
Sent: Monday, November 25, 2013 6:26:27 PM GMT +01:00 Amsterdam / Berlin / Bern / \
Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
Hi
I have a couple of high-level questions.
On 11/25/2013 08:28 PM, Mattias Tobiasson wrote:
</pre>
<blockquote>
<pre>Hi,
The test has been updated after the first review.
The two java files for each test has been merged to a single file.
Updated summary of changes:
1. The real test bug fix is to add flag "-Xshare:off" when starting the "Application" \
instance. Without that flag, the test for ClassFileTransformer in RedefineAgent.java \
fails intermittently. The flag is added in function startApplication() in \
RunnerUtil.java. </pre>
</blockquote>
<pre>What is the reason of this intermittent failure?
</pre>
<blockquote>
<pre>2. Removed all bash scripts.
</pre>
</blockquote>
<pre>Great!
</pre>
<blockquote>
<pre>3. Merged the setup bash script and the java test code for \
each test to a single java file. The old java test code is unchanged, it has just \
been moved to static nested class in the new java setup file.
4. Added some utility functions to jdk/testlibrary.
</pre>
</blockquote>
<pre>Here some comments about library code.
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Emiauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessThread.java.udiff.html" \
target="_blank">http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessThread.java.udiff.html</a>
+ try {
+ output = new OutputAnalyzer(this.process);
+ } catch (Throwable t) {
+ String name = Thread.currentThread().getName();
+ System.out.println(String.format("ProcessThread[%s] failed: %s", \
name, t.toString())); + throw t;
+ } finally {
+ String logMsg = ProcessTools.getProcessLog(processBuilder, output);
+ System.out.println(logMsg);
+ }
You used output in finally which is not initialized properly in the case
of Exception.
Could we get another uncaught exception in finally?
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Emiauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.udiff.html" \
target="_blank">http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.udiff.html</a>
1) Same in as previous in executeProcess method.
2) getCommandLine (...)
+ /**
+ * @return The full command line for the ProcessBuilder.
+ */
+ public static String getCommandLine(ProcessBuilder pb) {
+ StringBuilder cmd = new StringBuilder();
+ for (String s : pb.command()) {
+ cmd.append(s).append(" ");
+ }
+ return cmd.toString();
+ }
Should we also trim cmd to remove last " "?
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Emiauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/Utils.java.udiff.html" \
target="_blank">http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/Utils.java.udiff.html</a>
public static int waitForJvmPid(String key) throws Throwable {
Why it throw Trowable? Could we deal with exceptions in this method and
re-throw some more meaningful exception here?
Could you force flush for out here:
System.out.println("waitForJvmPid: Waiting for key '" + key + "'");
Hope it should be enough. It is scary to investigate such "timeouts".
Would it be better to add some
internal timeout in testlibrary? Really jtreg doesn't kill all processes
and we have alive java processes in such case.
For samevm tests timeouts are even worse. There is no good way to kill
test in samevm.
Should be
public static int tryFindJvmPid(String key) throws Throwable {
private? Are we going to use it in tests or only waitForJvmPid?
Leonid
</pre>
<blockquote>
<pre>5. Moved exit code check from the common utility function in \
ProcessThread.java to the test JstatdTest.java. The check is moved to the test \
because other tests in the future may have other expected exit codes.
Webrev:
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Emiauno/6461635/webrev.01/" \
target="_blank">http://cr.openjdk.java.net/~miauno/6461635/webrev.01/</a>
Bug:
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-6461635" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-6461635</a>
Mattias
----- Original Message -----
From: <a class="moz-txt-link-abbreviated" href="mailto:mattias.tobiasson@oracle.com" \
target="_blank">mattias.tobiasson@oracle.com</a>
To: <a class="moz-txt-link-abbreviated" href="mailto:mikael.auno@oracle.com" \
target="_blank">mikael.auno@oracle.com</a>
Cc: <a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net" \
target="_blank">serviceability-dev@openjdk.java.net</a>, <a \
class="moz-txt-link-abbreviated" href="mailto:Alan.Bateman@oracle.com" \
target="_blank">Alan.Bateman@oracle.com</a>
Sent: Thursday, November 21, 2013 9:22:11 AM GMT +01:00 Amsterdam / Berlin / Bern / \
Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
I agree that merging both files in a single file may be good.
The main reason why I did not do that is that I wanted to keep the history of the \
existing test. If I copy the test to a new file, all history of the code is lost.
But this test bug was reported 8 years ago, and I don't believe much has changed in \
the code since then. So keeping the history may not be that important.
Mattias
----- Original Message -----
From: <a class="moz-txt-link-abbreviated" href="mailto:mikael.auno@oracle.com" \
target="_blank">mikael.auno@oracle.com</a>
To: <a class="moz-txt-link-abbreviated" href="mailto:mattias.tobiasson@oracle.com" \
target="_blank">mattias.tobiasson@oracle.com</a>, <a class="moz-txt-link-abbreviated" \
href="mailto:Alan.Bateman@oracle.com" \
target="_blank">Alan.Bateman@oracle.com</a>
Cc: <a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net" \
target="_blank">serviceability-dev@openjdk.java.net</a>
Sent: Wednesday, November 20, 2013 4:11:45 PM GMT +01:00 Amsterdam / Berlin / Bern / \
Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
How about defining the class that you want to attach to as a static
inner class to the actual test? That would give you only one file, but
with two classes, each with its own main method and clear correlation
between them.
Mikael
On 2013-11-20 15:41, Mattias Tobiasson wrote:
</pre>
<blockquote>
<pre>Hi,
Each test requires 2 files, the actual test code and a helper file.
The helper file will launch a separate java process (called Application), and then \
start the actual test. The actual test will then attach to the Application.
For example:
PermissionTests.sh: Helper file that will launch Application instance and then start \
PermissionTest.java
PermissionTest.java: The actual test code that attaches to the Application.
It is the PermissionTests.sh that is started by jtreg (contains the "@test"-tag).
When I port PermissionTest.sh to java I would get 2 files called PermissionTest.java, \
so some name change is required.
I could have kept the old PermissionTest.java unchanged, but then I would need \
another name for the prevoius PermissionTest.sh. And I wanted a "clean" Test name for \
the file containing the "@test"-tag.
I used these names:
TestPermission.java: Helper file.
TestPermissionImpl.java: Actual test code.
I am still new to adding tests for open jdk. I am happy to change the names if they \
do not follow the naming convention.
Mattias
----- Original Message -----
From: <a class="moz-txt-link-abbreviated" href="mailto:Alan.Bateman@oracle.com" \
target="_blank">Alan.Bateman@oracle.com</a>
To: <a class="moz-txt-link-abbreviated" href="mailto:mattias.tobiasson@oracle.com" \
target="_blank">mattias.tobiasson@oracle.com</a>
Cc: <a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net" \
target="_blank">serviceability-dev@openjdk.java.net</a>
Sent: Wednesday, November 20, 2013 2:50:52 PM GMT +01:00 Amsterdam / Berlin / Bern / \
Rome / Stockholm / Vienna
Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
Out of curiosity, what is the reason for the rename? I ask because we
use Basic and similar names in many areas. Also anything in the test
tree should be a test.
-Alan.
On 20/11/2013 13:47, Mattias Tobiasson wrote:
</pre>
<blockquote>
<pre>Hi,
Could you please review this fix.
Summary of changes:
1. The real test bug fix is to add flag "-Xshare:off" when starting the "Application" \
instance. Without that flag, the test for ClassFileTransformer in RedefineAgent.java \
fails intermittently. The flag is added in function startApplication() in \
RunnerUtil.java.
2. Ported the following bash scripts to java:
BasicTests.sh -> TestBasic.java
PermissionTests.sh -> TestPermission.java
ProviderTests.sh -> TestProvider.java
3. Renamed the java test code to avoid name clash with new java classes ported from \
bash script: BasicsTest.java -> TestBasicImpl.java
PermissionTest.java -> TestPermissionImpl.java
ProviderTest.java -> TestProviderImpl.java
4. Added some utility functions to jdk/testlibrary.
5. Moved exit code check from the common utility function in ProcessThread.java to \
the test JstatdTest.java. The check is moved to the test because other tests in the \
future may have other expected exit codes.
Thanks,
Mattias
Webrev:
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Emiauno/6461635/webrev.00/" \
target="_blank">http://cr.openjdk.java.net/~miauno/6461635/webrev.00/</a>
Bug:
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-6461635" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-6461635</a> </pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<br>
<br>
<pre class="moz-signature">--
Leonid Mesnik
JVM SQE</pre>
</div>
</div>
</blockquote>
<br>
<br>
<pre class="moz-signature">--
Leonid Mesnik
JVM SQE</pre>
</div>
</div></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic