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

[Dmitry Dmitriev]

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().

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?

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?

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.

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

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

Thank you!

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