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.mes...@oracle.com
To: mattias.tobias...@oracle.com, david.hol...@oracle.com
Cc: serviceability-dev@openjdk.java.net, daniel.daughe...@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 <https://bugs.openjdk.java.net/browse/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.hol...@oracle.com
To:mattias.tobias...@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.mes...@oracle.com
To:mattias.tobias...@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.mes...@oracle.com
To:mattias.tobias...@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.tobias...@oracle.com
To:mikael.a...@oracle.com
Cc:serviceability-dev@openjdk.java.net,alan.bate...@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.a...@oracle.com
To:mattias.tobias...@oracle.com,alan.bate...@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.bate...@oracle.com
To:mattias.tobias...@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
