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

Remove finalization from RegistryKey #8488

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Conversation

basil
Copy link
Member

@basil basil commented Sep 13, 2023

Recent versions of Java have deprecated Object#finalize and marked it for removal. In this PR we are removing finalization from RegistryKey, which already implemented explicit termination via AutoCloseable. Unlike #8486, there were only a handful of usages throughout the ecosystem (I checked the cloudbees GitHub organization as well), and I audited each and every one to ensure it was already using the explicit terminator, either via try-with-resources or try-finally. Furthermore, all usages outside core were in extremely ancient plugins that are not relevant anymore. For core itself, SpotBugs would warn us if we introduced a new resource leak by forgetting to use try-with-resources. Weighing all of this against the complexity of reimplementing this (untested) functionality with the Cleaner API, it seemed best to simply dispense with this unused fallback mechanism rather than to port it to Java 9+.

Testing done

mvn clean verify -DskipTests

Proposed changelog entries

  • Entry 1: Issue, human-readable text
  • […]

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

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

Maintainer checklist

Preview Give feedback

@basil basil added the skip-changelog Should not be shown in the changelog label Sep 13, 2023
@NotMyFault NotMyFault requested a review from a team September 16, 2023 15:45
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks!

@MarkEWaite MarkEWaite 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 16, 2023
@MarkEWaite
Copy link
Contributor

MarkEWaite commented Sep 16, 2023

This PR is now ready for merge. We will merge it after the release of the next weekly Jenkins version if there is no negative feedback. Merges to the master branch are intentionally blocked in preparation for the upcoming security release https://groups.google.com/g/jenkinsci-advisories/c/-RZfZDf9srA

@MarkEWaite MarkEWaite self-assigned this Sep 16, 2023
@NotMyFault NotMyFault merged commit b7920ec into jenkinsci:master Sep 20, 2023
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.

3 participants