RFR: 8273969: Memory Leak on the Lambda provided to Platform.startup

[Kevin Rushforth]

On Sun, 19 Sep 2021 15:33:46 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

> Probably my most boring PR. ;)
> Setting the lambda to null, after it has been used, fixes the leak.

The fix looks obviously correct. I verified that the test catches the bug (fails without the fix and passes with the fix). I left a few cleanup comments on the test.

Btw, I almost left an additional test comment about needing to wait for the `Runnable` to be called (e.g., by waiting for a latch that is triggered in the `Runnable`), but I realized that this is unnecessary given that the test waits for the `Runnable` to be garbage-collected, which can't happen until after the `run` method of the `Runnable` has been called.

One last suggestion is that I recommend to change "Lambda" to "Runnable" in the title of the bug (and thus the PR).

tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java line 26:

> 24:  */
> 25: 
> 26: package test.javafx.scene;

Similar tests are in the `test.com.sun.javafx.application` package, so I recommend putting this test there.

tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java line 34:

> 32: public class PlatformStartupMemoryLeakTest {
> 33: 
> 34:     @Test

Since this test calls `Platform::startup`, this class must only have a single `@Test` method. I recommend adding a comment to that effect, so that someone doesn't try to add a second test method later.

tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java line 37:

> 35:     public void testStartupLeak() {
> 36:         JMemoryBuddy.memoryTest((checker) -> {
> 37:             // Doesn't work as lambda for some reason, due to "BoundMethodHandle$Species_L"

Based on Tom's comment, and also my reading of the spec, this is expected (if somewhat counter-intuitive), so please modify this comment to indicate that the `Runnable` must not be turned into a lambda.

tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java line 47:

> 45:             checker.assertCollectable(r);
> 46:         });
> 47:     }

I recommend adding an `@After` (or `@AfterClass`) cleanup method that calls `Platform::exit`.

-------------

PR: https://git.openjdk.java.net/jfx/pull/626