RFR: 8273969: Memory Leak on the Runnable provided to Platform.startup [v2]

[Florian Kirmaier]

On Mon, 20 Sep 2021 12:56:39 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   JDK-8273969
>>   Some changes based on code review
>
> 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.

done

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

I think that's redundant after moving it to the application package. 
In the application package, most of the test classes only have 1 test for this reason.

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

done

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

done

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

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