On 27 nov 2013, at 12:21, David Holmes <[email protected]> wrote:
> 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.
There is nothing in the test that expects class data sharing to be tested. The
test is a basic verification of the attach mechanism.
As far as I can tell, It is expected that ClassFileTransformer.transform() is
not called for classes in the shared archive (BigInteger) in this case. Thus, I
think it is a good idea to either use a different class or to explicitly
disable class data sharing (simpler).
/Staffan
>
> 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: [email protected]
>> To: [email protected]
>> Cc: [email protected]
>> 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: [email protected]
>>> To: [email protected], [email protected]
>>> 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: [email protected]
>>>> To: [email protected]
>>>> Cc: [email protected], [email protected]
>>>> 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: [email protected]
>>>> To: [email protected], [email protected]
>>>> Cc: [email protected]
>>>> 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: [email protected]
>>>>> To: [email protected]
>>>>> Cc: [email protected]
>>>>> 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
>>>
>>
>>
