Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from deprecated finalization API to Java 9+ API #8486

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

basil
Copy link
Member

@basil basil commented Sep 13, 2023

Recent versions of Java have deprecated Object#finalize and marked it for removal:

[WARNING] AtomicFileWriter.java:[229,19] [removal] finalize() in Object has been deprecated and marked for removal
[WARNING] AtomicFileWriter.java:[233,17] [removal] finalize() in Object has been deprecated and marked for removal

In this PR we proactively migrate away from this deprecated API in favor of its Java 9+ replacement.

As a bonus, we also implement the advice given by Joshua Bloch in Effective Java (emphasis not mine):

So what, if anything, are finalizers good for? […] One is to act as a “safety net” in case the owner of an object forgets to call its explicit termination method. While there’s no guarantee that the finalizer will be invoked promptly, it may be better to free the resource late than never, in those (hopefully rare) cases when the client fails to call the explicit termination method. But the finalizer should log a warning if it finds that the resource has not been terminated, as this indicates a bug in the client code, which should be fixed.

We also add test coverage for this functionality. The new test passes before the changes to src/main, demonstrating there is no regression.

Testing done

Ran AtomicFileWriterTest on Java 17 on both Linux and Windows.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

@basil basil added the skip-changelog Should not be shown in the changelog label Sep 13, 2023
Path tmpPath = ref.get().getTemporaryPath();
assertTrue(Files.exists(tmpPath));
assertFalse(Files.exists(destPath));
MemoryAssert.assertGC(ref);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a separate test in test/ rather than in core/src/test/ to be able to have access to MemoryAssert.

@NotMyFault NotMyFault requested a review from a team September 16, 2023 15:45
@NotMyFault NotMyFault requested a review from a team September 17, 2023 14:38
@timja
Copy link
Member

timja commented Sep 17, 2023

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 17, 2023
@NotMyFault NotMyFault removed the request for review from a team September 20, 2023 18:42
@NotMyFault NotMyFault merged commit febca34 into jenkinsci:master Sep 20, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants