On Sun, 19 Sep 2021 15:33:46 GMT, Florian Kirmaier <fkirma...@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
