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 Runtime.runFinalizersOnExit() in upgrade to Java 11 #244

Merged
merged 8 commits into from
Nov 20, 2023

Conversation

satvika-eda
Copy link
Contributor

@satvika-eda satvika-eda commented Jun 27, 2023

What's changed?

Runtime.runFinalizersOnExit() has been removed and replaced in latest JDK versions.

What's your motivation?

To automate JDK migration to a greater extent

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@timtebeek timtebeek added the recipe Recipe requested label Jun 27, 2023
@timtebeek timtebeek self-requested a review June 27, 2023 10:09
@timtebeek
Copy link
Contributor

For context: https://bugs.openjdk.org/browse/JDK-8198250

Some of the same review comments from #246 and #247 apply here, such as adding tags, markdown, removing markers. Could you apply those same changes here?

Thanks again; really neat to see so many pull requests opened at once!

"""
class FooBar {
public void test(){
Runtime.getRuntime().addShutdownHook(new Thread());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I lack some background on why we inject addShutdownHook(new Thread()) here; do you have a source to explain why that's needed? Seems strange to have an empty Thread run yet still provide value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also like some context here. Can't the method call simply be removed?

Copy link
Contributor Author

@satvika-eda satvika-eda Jul 13, 2023

Choose a reason for hiding this comment

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

Made the following changes : Runtime.getRuntime().addShutdownHook(new Thread(() -> Runtime.getRuntime().exit(0))); to recipe to exit the JVM as per https://bugs.openjdk.org/browse/JDK-8198250. Let me know your suggestions.

@Getter
public class ReplaceRuntimeFinalizer extends Recipe {

private static final String METHOD_PATTERN = "runFinalizersOnExit";
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more idiomatic to add two MethodMatchers for java.lang.Runtime runFinalizersOnExit(boolean) and java.lang.System runFinalizersOnExit(boolean). That has the advantage that they will also match if these methods were imported statically.

"""
class FooBar {
public void test(){
Runtime.getRuntime().addShutdownHook(new Thread());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also like some context here. Can't the method call simply be removed?

@satvika-eda satvika-eda mentioned this pull request Jul 11, 2023
4 tasks
@timtebeek timtebeek requested a review from joanvr July 13, 2023 09:40
@joanvr joanvr added the question Further information is requested label Jul 27, 2023
Copy link
Contributor

@joanvr joanvr left a comment

Choose a reason for hiding this comment

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

Looking at the referenced documents, I do not understand why we are replacing the method calls for this replacements, instead of just removing them... Maybe you could provide us more context?

@timtebeek timtebeek changed the title replace runtime finalizer recipe Remove Runtime.runFinalizersOnExit() in upgrade to Java 11 Nov 20, 2023
@timtebeek
Copy link
Contributor

Thanks again @satvika-eda ! I've taken the liberty to use your PR to clean up some duplication we had around removing existing method calls. All of that's now reusable from properties, which should help if there's future such cases.

@timtebeek timtebeek merged commit 1349e83 into openrewrite:main Nov 20, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants