Skip to content

[JENKINS-75443] Removed deprecated finalize methods in favor of Cleaner #794

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.

Sign up for GitHub

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

Jump to bottom
Open
akash-manna-sky wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[JENKINS-75443] Removed deprecated finalize methods in favor of Cleaner #794

akash-manna-sky wants to merge 1 commit into from

Conversation

@akash-manna-sky
Copy link

@akash-manna-sky akash-manna-sky commented Mar 27, 2025 •
edited
Loading

JENKINS-75443

Fix #1509

I added Cleaner for resource management in FastPipedInputStream, FastPipedOutputStream, ProxyOutputStream, and ProxyWriter

  • Introduced Cleaner to manage resource cleanup on garbage collection.
  • Updated constructors to register cleanup actions.
  • Removed deprecated finalize methods in favor of Cleaner.
  • Improved code formatting and documentation for clarity.

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@akash-manna-sky
Copy link
Author

akash-manna-sky commented Apr 10, 2025

Hello @basil Please review the changes

@akash-manna-sky akash-manna-sky marked this pull request as ready for review April 10, 2025 11:04
@akash-manna-sky akash-manna-sky force-pushed the issue-75443 branch 2 times, most recently from 8befaec to fa86fa1 Compare April 14, 2025 15:17
@timja timja added enhancement For changelog: An enhancement providing new capability. developer and removed enhancement For changelog: An enhancement providing new capability. labels Apr 15, 2025
timja
timja reviewed Apr 15, 2025
View reviewed changes
timja
timja reviewed Apr 15, 2025
View reviewed changes
@akash-manna-sky akash-manna-sky force-pushed the issue-75443 branch 2 times, most recently from c7faf21 to b85076c Compare April 15, 2025 15:14
@akash-manna-sky
Copy link
Author

akash-manna-sky commented Apr 15, 2025

@timja I make few changes. Please review the changes.

@timja
Copy link
Member

timja commented Apr 15, 2025

I don't know anything about this area so am uncomfortable reviewing this right now.

Thanks for the pull request though.

@akash-manna-sky akash-manna-sky force-pushed the issue-75443 branch 2 times, most recently from 1d636d6 to 0cf381b Compare April 22, 2025 18:07
@akash-manna-sky
Copy link
Author

akash-manna-sky commented Apr 28, 2025

Hello @basil & @jglick
Please let me know if there’s anything I need to update or improve to help move it forward. Looking forward to your feedback.
Thanks

jglick
jglick reviewed Apr 28, 2025
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

The Proxy* changes look right from a quick glance but the FastPiped*Stream changes do not. Not sure, I do not have much experience with Cleaner and it seems very prone to misuse, especially if there is no test coverage for the existing finalize methods (as I suspect).

Refactoring suggested by @basil in the context of jenkinsci/jenkins#8486 FTR.

@jglick jglick added enhancement For changelog: An enhancement providing new capability. and removed developer labels Apr 28, 2025
@jglick
Copy link
Member

jglick commented Apr 29, 2025

or #796

@akash-manna-sky
Copy link
Author

akash-manna-sky commented Apr 29, 2025

or #796

PR #796 also addresses the same issue. Just to clarify, should I continue working on my PR, or would you prefer I refer to PR #796 and align with that approach? Please let me know.

@akash-manna-sky
Copy link
Author

akash-manna-sky commented May 1, 2025

@jglick I make few changes as you requested. Please review the changes.

Thanks

@jglick
Copy link
Member

jglick commented May 2, 2025

I will add both to my review backlog but it might take a while. These also probably need testing in bom to check for regressions, since these classes are used in every Jenkins build.

@akash-manna-sky
Copy link
Author

akash-manna-sky commented May 9, 2025

Hello @jglick
Have you had a chance to review the changes? Please let me know if there's anything I can do to help.

Thanks for your time!

@akash-manna-sky akash-manna-sky force-pushed the issue-75443 branch 2 times, most recently from 5b23f28 to 2df8845 Compare May 18, 2025 07:03
@akash-manna-sky akash-manna-sky requested a review from jglick May 20, 2025 06:44
@akash-manna-sky
Copy link
Author

akash-manna-sky commented Jun 17, 2025

Hello @jglick,
Any review updates about this PR?

@akash-manna-sky akash-manna-sky requested a review from timja July 29, 2025 16:43
@akash-manna-sky akash-manna-sky force-pushed the issue-75443 branch 2 times, most recently from d4a58bd to 2af12b8 Compare October 9, 2025 12:14
@akash-manna-sky
Copy link
Author

akash-manna-sky commented Dec 6, 2025

Any updates on this PR? Could you please review this PR?
@jglick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jglick jglick Awaiting requested review from jglick

@timja timja Awaiting requested review from timja

Assignees

No one assigned

Labels

enhancement For changelog: An enhancement providing new capability.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[JENKINS-75443] Migrate from deprecated finalization API

3 participants

@akash-manna-sky @timja @jglick