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
>>
>
>
