RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
Leonid Mesnik
leonid.mesnik at oracle.com
Thu Nov 28 02:01:03 PST 2013
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 at oracle.com
> To: mattias.tobiasson at oracle.com, david.holmes at oracle.com
> Cc: serviceability-dev at openjdk.java.net, daniel.daugherty at 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.holmes at oracle.com
> To:mattias.tobiasson at oracle.com
> Cc:serviceability-dev at 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 at oracle.com
> To:mattias.tobiasson at oracle.com
> Cc:serviceability-dev at 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 at oracle.com
> To:mattias.tobiasson at oracle.com,serviceability-dev at 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 at oracle.com
> To:mikael.auno at oracle.com
> Cc:serviceability-dev at openjdk.java.net,Alan.Bateman at 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 at oracle.com
> To:mattias.tobiasson at oracle.com,Alan.Bateman at oracle.com
> Cc:serviceability-dev at 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 at oracle.com
> To:mattias.tobiasson at oracle.com
> Cc:serviceability-dev at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20131128/19c6d3fb/attachment-0001.html
More information about the serviceability-dev
mailing list