Clean caches upon deletion of a Target Definition #1246

[basilevs]

TargetPlatformHelper and TargetPlatformService keep separate caches for target definition, resulting in a leak of TargetDefinition objects.

The leak happens when TargetDefinition.resolve() caches data in TargetPlatformHelper.

Fixes #1246

[github-actions]

Test Results

   291 files  ±0     291 suites  ±0   53m 56s ⏱️ - 1m 55s
 3 526 tests ±0   3 468 ✅ ±0   58 💤 ±0  0 ❌ ±0 
10 875 runs  ±0  10 698 ✅ ±0  177 💤 ±0  0 ❌ ±0 

Results for commit 145a426. ± Comparison against base commit a42910f.

♻️ This comment has been updated with latest results.

[HannesWell]

Thank you @basilevs for this fix.
In general it looks reasonable to me, but after a quick check of the callers I think we have to check in detail if TargetPlatformPreferencePage.handleRemove/performOk() needs adjustments to ensure a target is not prematurely removed from the map.
I can can help with that this evening.

[HannesWell]

In general it looks reasonable to me, but after a quick check of the callers I think we have to check in detail if TargetPlatformPreferencePage.handleRemove/performOk() needs adjustments to ensure a target is not prematurely removed from the map.

I checked the preference-page in detail and the call to TargetPlatformHelper.getTargetDefinitionMap().remove() in TargetPlatformPreferencePage.handleRemove can be removed since it will be called in performOk() with the removed targets when TargetPlatformService.deleteTarget() is called.

I applied that already so that this change can be submitted as soon as the build completes.

Thank you for this contribution.

[basilevs]

since it will be called in /performOk() with the removed targets when TargetPlatformService.deleteTarget() is called.

Not for Cancel?

[HannesWell]

since it will be called in /performOk() with the removed targets when TargetPlatformService.deleteTarget() is called.

Not for Cancel?

Yes if the dialog is canceled deleteTarget() is not called but also the target is not deleted. Since handleRemove() only adds a target to the list of targets to be removed but does not yet delete them. So deleting it from the other cache at that point in time was actually too early since as discovered, the target is not deleted if Cancel is pressed.
Therefore moving the removal from the TargetPlatformHelper-cache to the time when the deletion is really performed actually also fixes that 'bug'.

[HannesWell]

Btw. the new test fails in I-builds:
https://download.eclipse.org/eclipse/downloads/drops4/I20240518-1800/testresults/html/org.eclipse.pde.ui.tests_ep432I-unit-win32-java17_win32.win32.x86_64_17.html

[HannesWell]

Btw. the new test fails in I-builds: https://download.eclipse.org/eclipse/downloads/drops4/I20240518-1800/testresults/html/org.eclipse.pde.ui.tests_ep432I-unit-win32-java17_win32.win32.x86_64_17.html

For some of the test-classes the test cases are not found and zero tests are executed ...

Running AllPDETests org.eclipse.pde.ui.tests.target.AllTargetTests org.eclipse.pde.ui.tests.target.LocalTargetDefinitionTests
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.003 s -- in AllPDETests org.eclipse.pde.ui.tests.target.AllTargetTests org.eclipse.pde.ui.tests.target.LocalTargetDefinitionTests
Running AllPDETests org.eclipse.pde.ui.tests.target.AllTargetTests org.eclipse.pde.ui.tests.target.WorkspaceTargetDefinitionTests
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0 s -- in AllPDETests org.eclipse.pde.ui.tests.target.AllTargetTests org.eclipse.pde.ui.tests.target.WorkspaceTargetDefinitionTests

I'll investigate that.

[HannesWell]

For some of the test-classes the test cases are not found and zero tests are executed ...

Running AllPDETests org.eclipse.pde.ui.tests.target.AllTargetTests org.eclipse.pde.ui.tests.target.LocalTargetDefinitionTests
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.003 s -- in AllPDETests org.eclipse.pde.ui.tests.target.AllTargetTests org.eclipse.pde.ui.tests.target.LocalTargetDefinitionTests
Running AllPDETests org.eclipse.pde.ui.tests.target.AllTargetTests org.eclipse.pde.ui.tests.target.WorkspaceTargetDefinitionTests
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0 s -- in AllPDETests org.eclipse.pde.ui.tests.target.AllTargetTests org.eclipse.pde.ui.tests.target.WorkspaceTargetDefinitionTests

I'll investigate that.

Looks like I disabled those tests in #745 (comment) since they have to be reworked to be able to run in I-builds.

eclipse.pde/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/target/LocalTargetDefinitionTests.java

Lines 75 to 77 in e41da13

	public static void requireStandaloneEclipseSDKEnvironment() {
	PDETestCase.assumeRunningInStandaloneEclipseSDK();
	}

I'll have a look at it.