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

[ISSUE #4098] Method may fail to clean up stream or resource[WatchFileManagerTest] #4274

Closed
wants to merge 6 commits into from

Conversation

kyooosukedn
Copy link
Contributor

@kyooosukedn kyooosukedn commented Jul 22, 2023

Fixes #4098.

Motivation

Method may fail to clean up stream or resource
Explain why you want to make the changes and what problem you're trying to solve.

Modifications

Used try with resources to manage resources

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

properties.setProperty("eventMesh.server.newAdd", "newAdd");
properties.store(fw, "newAdd");
} catch (IOException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Please use log4j instead.

Comment on lines 53 to 55
try (FileReader fr = new FileReader(file);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(file)) {
Copy link
Member

@Pil0tXia Pil0tXia Jul 23, 2023

Choose a reason for hiding this comment

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

Good job.

When using .close() to close nested streams, we only need to close the outermost stream, because the close request propagates from the outermost stream to the innermost stream.

To have try-with-resources close them, you assigned the streams to variables as you opened them, instead of nesting. If you used nesting, an exception during construction of one of the later streams (e.g. GZIPOutputStream) will leave any stream constructed by the nested calls inside it open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review :)

Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

@kyooosukedn Test case failed, Pls fix it

@Alonexc
Copy link
Contributor

Alonexc commented Jul 25, 2023

Ref:

try (final BufferedReader reader = new BufferedReader(new FileReader(file));
             final FileWriter fw = new FileWriter(file)) {
            Properties properties = new Properties();
            properties.load(reader);
            properties.setProperty("eventMesh.server.newAdd", "newAdd");
            properties.store(fw, "newAdd");
        }

@xwm1992 xwm1992 changed the title [Enhancement] Method may fail to clean up stream or resource[WatchFileManagerTest] #4098 [ISSUE #4098 ] Method may fail to clean up stream or resource[WatchFileManagerTest] Jul 27, 2023
@xwm1992 xwm1992 changed the title [ISSUE #4098 ] Method may fail to clean up stream or resource[WatchFileManagerTest] [ISSUE #4098] Method may fail to clean up stream or resource[WatchFileManagerTest] Jul 27, 2023
@Alonexc
Copy link
Contributor

Alonexc commented Jul 27, 2023

image
please check checkstyle.

@Pil0tXia
Copy link
Member

Hi @kyooosukedn , the issue has been accomplished in #4716. Do you have any additional information to share? Feel free to share your latest insights with us. If you feel that your issue has been resolved, you are welcome to close it. We sincerely appreciate your contributions to Apache EventMesh.

@kyooosukedn
Copy link
Contributor Author

It seems like the issue is closed properly. I will close this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Method may fail to clean up stream or resource[WatchFileManagerTest]
4 participants