Switch to the Cleaner API

[otbutz]

Fixes #1465

• replaced the current implementation that relies on finalize() by using a Cleaner instead
• the ResourceTracker no longer holds a reference to the resource, as the Cleaner API mandates that:

  The cleaning action is invoked only after the associated object becomes phantom reachable, so it is important that the object implementing the cleaning action does not hold references to the object.

• a flag is now used to detect whether the nonDisposedReporter should be invoked.
• the tracker is registered during init() to avoid false-negatives for resources throwing an exception in their constructor. e.g new Cursor(display, -100)

[github-actions]

Test Results

   545 files  + 6     545 suites  +6   33m 25s ⏱️ + 8m 19s
 4 367 tests +37   4 351 ✅ +35   16 💤 +3  0 ❌  - 1 
16 616 runs  +37  16 502 ✅ +35  114 💤 +3  0 ❌  - 1 

Results for commit a505cf1. ± Comparison against base commit 536fea8.

♻️ This comment has been updated with latest results.

[iloveeclipse]

@SyntevoAlex : are you interested in a review of this PR?

[SyntevoAlex]

Please proceed without me.

[HannesWell]

I can have a look at this in the next days, sorry for the delayed reply.

[jonahgraham]

I think it is great to see the removal of finalize. The not disposed reporting has been really useful. Having it be ready for the future when finalize is removed is the right step forward. Thanks for taking this on.

I have added some small suggestions/commentary.

[otbutz]

@jonahgraham I think the ResourceTrackerThreadFactory should be close enough to what the InnocuousThreadFactory does.

[jonahgraham]

I have tried out the patch on Linux and it looks good to me. I tested it both with the manual test org.eclipse.swt.tests.manual.Bug569752_DetectNonDisposedOsResources and by running the IDE (with a couple of dispose calls commented out to see the reporting in operation - and heap status on so I can trigger GC)

This looks good to me. As this code sits so completely core to everything SWT I would prefer if someone else approved too before it gets merged.

@jonahgraham I think the ResourceTrackerThreadFactory should be close enough to what the InnocuousThreadFactory does.

It looks like it to me.

[otbutz]

Sorry for the last minute change. I've moved the cleaner from the Resource class to the ResourceTracker class. The advantage of this is that the cleaner thread is not started until the first instance of a ResourceTracker is created, as specified in JLS §12.4.1

[jonahgraham]

Sorry for the last minute change. I've moved the cleaner from the Resource class to the ResourceTracker class. The advantage of this is that the cleaner thread is not started until the first instance of a ResourceTracker is created, as specified in JLS §12.4.1

I did a recheck with the latest commit. It looks fine to me.

[otbutz]

Missed a call to Cleanable.clean() within ignoreNonDisposed().

Edit: This should have no effect, since the only caller of this method never calls init() in the first place. No idea why unit tests fail now 🤷‍♂️

[HannesWell]

This looks good to me. As this code sits so completely core to everything SWT I would prefer if someone else approved too before it gets merged.

I just had a brief look and generally this looks sane, but I can have a detailed second review tomorrow.

@otbutz is this an urgent change for you that should be available for the March release? If not we could postpone the submission to the beginning of the next release cycle to have more time to find potential, unexpected drawbacks.

In general @otbutz could you please squash all commits into one with a commit message that covers and if necessary explains the overall change?

[otbutz]

Feel free to postpone. The current finalize-based implementation works fine, and it would be unwarranted to rush a replacement that hasn't been properly vetted.

[iloveeclipse]

I've only rebased on latest SWT state.

[iloveeclipse]

LGTM.