RFR(S): 8173743: Failures during class definition can lead to memory leaks in metaspace

Thu Feb 9 09:10:37 UTC 2017

Hello Volker,

Looks good to me! Thank you.

Dmitry

On 08.02.2017 22:42, Volker Simonis wrote:
> Hi Dmitry,
>
> thanks for looking at my change and your valuable comments. I hope
> I've addressed them all. Please find my detailed comments inline:
>
> On Tue, Feb 7, 2017 at 11:53 AM, Dmitry Dmitriev
> <dmitry.dmitriev at oracle.com> wrote:
>> Hello Volker,
>>
>> Few additional test comments:
>> 1) 168         String self = ManagementFactory.getRuntimeMXBean().getName();
>>   169         String pid = self.substring(0, self.indexOf('@'));
>>   170         System.out.println("Our pid is = " + pid);
>>
>> I think it's is better to use getProcessId() method from
>> jdk.test.lib.process.ProcessTools testlibrary
>> package(test/lib/jdk/test/lib/process/ProcessTools.java) to get pid().
>>
> Sure. I've first written this as a stand alone test, but now that it
> is a JTreg test I can change it.
>
>> 2)  214             Scanner s = new Scanner(result);
>>
>> Can you please put code for Scanner in the try-with-resources block and
>> remove 's.close();' on line 226?
>>
> Done.
>
>> 3) In case of error getClassStats() prints information about exception and
>> returns 0. Test will fail later at this check "if (count != ...) throw new
>> RuntimeException(res);". Probably test should fail(raise an exception) in
>> getClassStats() method in case of the error to avoid confusion?
>>
> Good point. Done.
>
>> 4) Can you please move "(args.length > 1 ? Integer.parseInt(args[1]) : 10)"
>> outside of the for loop in lines 243,277,304,362 to improve readability?
>> I.e. do something like that:
>> static final int DEFAULT_DEFINE_ATTEMPTS = 10;
>> ...
>> public static void main(String[] args) throws Exception {
>>      int defineAttempts = args.length > 1 ? Integer.parseInt(args[1]) :
>> DEFAULT_DEFINE_ATTEMPTS;
>> ...
>>      for (int i = 0; i < defineAttempts; i++) {
>>
>> In this case 'defineAttemps' also can be used on lines 332,333 and 'length'
>> can be removed.
>>
> Done.
>
>> 5)  338                 Thread.sleep(100); // Give the new thread a chance
>> to start
>>
>> I don't feel happy with sleep :) For 10 threads we get 1 second sleep in
>> total. It is possible to use different mechanism to ensure that threads are
>> start(another CountDownLatch)?
>>
> Done.
>
>> 6) I think that you can reduce some code duplication in the main method by
>> moving following construction to the separate method:
>>   261             count = getClassStats("DefineClass");
>>   262             // At least after System.gc() the failed loading attempts
>> should leave no instances around!
>>   263             String res = "Should have 2 DefineClass instances and we
>> have: " + count;
>>   264             System.out.println(res);
>>   265             if (count != 2) throw new RuntimeException(res);
>>
>> E.g.
>>
>> void printClassStats(int expectedCount, boolean reportError) {
>>      count = getClassStats("DefineClass");
>>      String res = "Should have " + expectedCount + " DefineClass instances
>> and we have: " + count;
>>      System.out.println(res);
>>      if (reportError && count != expectedCount) {
>>          throw new RuntimeException(res);
>>      }
>> }
>>
>> In this case following piece of code:
>>   254             int count = getClassStats("DefineClass");
>>   255             // We expect to have two instances of DefineClass here: the
>> initial version in which we are
>>   256             // executing and one another version which was loaded into
>> our own classloader 'MyClassLoader'.
>>   257             // All the subsequent attempts to reload DefineClass into
>> our 'MyClassLoader' should have failed.
>>   258             System.out.println("Should have 2 DefineClass instances and
>> we have: " + count);
>>   259             System.gc();
>>   260             System.out.println("System.gc()");
>>   261             count = getClassStats("DefineClass");
>>   262             // At least after System.gc() the failed loading attempts
>> should leave no instances around!
>>   263             String res = "Should have 2 DefineClass instances and we
>> have: " + count;
>>   264             System.out.println(res);
>>   265             if (count != 2) throw new RuntimeException(res);
>>
>> will be transformed to the following:
>>      // We expect to have two instances of DefineClass here: the initial
>> version in which we are
>>      // executing and one another version which was loaded into our own
>> classloader 'MyClassLoader'.
>>      // All the subsequent attempts to reload DefineClass into our
>> 'MyClassLoader' should have failed.
>>      printClassStats(2, false);
>>      System.gc();
>>      System.out.println("System.gc()");
>>      // At least after System.gc() the failed loading attempts should leave
>> no instances around!
>>      printClassStats(2, true);
>>
> Yes, that's a nice refactoring! Done.
>
>> Thank you!
>>
> I've uploaded a new webrev which contains the test changes for all
> suggestions I've received until now:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8173743.v2/
>
> Thanks,
> Volker
>
>> Dmitry
>>
>>
>> On 06.02.2017 21:40, Volker Simonis wrote:
>>> Hi,
>>>
>>> can somebody please review the following change:
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8173743.v1/
>>> https://bugs.openjdk.java.net/browse/JDK-8173743
>>>
>>> It fixes some problems during class definitions where instance klasses
>>> can leak into the metaspace in cases where the class definition fails
>>> after the class was successfully loaded from the bytecode stream.
>>>
>>> There are actually two cases to consider:
>>>
>>> 1. If we load the same class several times into the same class loader,
>>> we will get LinkageErrors after the first successful load. But
>>> nevertheless we will first construct a new instanceKlass for each new
>>> load attempt and this instanceKlass will never be deleted.
>>>
>>> 2. If we have a parallel capable class loader, set
>>> -XX:-UnsyncloadClass and/or -XX:+AllowParallelDefineClass and load a
>>> class from several threads at the same time in parallel, it can happen
>>> that we create several instance klasses for the same class. At the end
>>> only one of them will be added to the system dictionary, but all the
>>> other ones will never be deleted. Notice that if we run this scenario
>>> without setting either of -XX:-UnsyncloadClass or
>>> -XX:+AllowParallelDefineClass, this scenario will degrade into the
>>> case above and we will get LinkageErrors for all but the first
>>> successful load.
>>>
>>> The change comes with a regression test which checks for the two cases
>>> just describe and also for the failing class redefinition case, which
>>> currently doesn't produce a memory leak.
>>>
>>> I've already committed this to the hs-demo-submit/hotspot/ forest and
>>> it went through without a problem. So in theory it should have passed
>>> the internal JPRT tests although I'm not sure if the test set of the
>>> "demo-submit" forest and the real hotspot repo are exactly the same
>>> (CC'ed Tim and Brian).
>>>
>>> Thank you and best regards,
>>> Volker
>>