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

[David Holmes]

Hi Volker,

On 7/02/2017 4:40 AM, 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

Functional change looks good - took me a while to work through the 
permutations. :)

Have a few comments on the test, mostly nits.

I believe an Oracle copyright line is also required.

linkageErrors is not incremented in a thread-safe way, and should also 
be volatile. I think you are just using this as a rough count, but if so 
document that and at least declare it volatile so it is reported with a 
current value if needed.

256             // executing and one another version

Either: "one other version" or "another version" (occurs elsewhere)

  283                 catch (java.lang.SecurityException jlse) {
  284                     // Expected
  285                 }

If the SecurityException is always expected then should not absence of 
it be reported as an error?

288             // We expect to have two instances of DefineClass here: 
the initial version in which we are
...
291             System.out.println("Should have 1 DefineClass instances 
and we have: " + count);

Comment and code do not match: do we expect one instance or two?

  292             System.gc();
  293             System.out.println("System.gc()");
  294             count = getClassStats("DefineClass");
  295             // At least after System.gc() the failed loading 
attempts should leave no instances around!

Are we guaranteed, for all GC's, that a single call to System.gc(), will 
clear the unwanted instances? (This applies to all test cases.)

  353             // kept alive by this main methidf

Typo: methidf

367                 catch (UnsupportedOperationException uoe) {
368                     // Expected
369                 }

Ditto re not getting the exception - should this report an error?

Thanks,
David
------

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