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

[Volker Simonis]

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