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

IOUtils.closeQuietly is deprecated and should be replaced with try-with-resource #1215

Closed
emmebi opened this issue Feb 1, 2020 · 12 comments
Closed
Assignees
Labels
code-maintenance Adding/editing javadocs, unit tests, formatting.

Comments

@emmebi
Copy link
Collaborator

emmebi commented Feb 1, 2020

Is your feature request related to a problem? Please describe.
IOUtils.closeQuietly has been deprecated and is marked as such in the code. The alternative is to use try-with-resources statement, which will remove some repetitive code.

The reference documentation is here: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

Describe the solution you'd like
Remove all the try/finally blocks of code including IOUtils.closeQuietly with try-with-resource statement

Describe alternatives you've considered
I am not aware of alternative ways to accomplish this.

Additional context
I am looking at blocks of code like this:

  public static Object objFromResource(String res) throws IOException {
    XStream xs = getConfiguredXStream();
    InputStream is = null;
    try {
      is = FileUtil.class.getClassLoader().getResourceAsStream(res);
      return xs.fromXML(new InputStreamReader(is, "UTF-8"));
    } finally {
      IOUtils.closeQuietly(is);
    }
  }

which can now be rewritten as:

  public static Object objFromResource(String res) throws IOException {
    XStream xs = getConfiguredXStream();
   try (
        InputStream is =  FileUtil.class.getClassLoader().getResourceAsStream(res);
   ) {
      return xs.fromXML(new InputStreamReader(is, "UTF-8"));
    }
  }

The same pattern could be applied also to other places, like for example PersistenceUtils.saveCampaignProperties

@emmebi
Copy link
Collaborator Author

emmebi commented Feb 1, 2020

On the same topic, the PackedFile could declare to implement the Autoclosable interface, and be used in the same way.

@Phergus Phergus added the code-maintenance Adding/editing javadocs, unit tests, formatting. label Feb 1, 2020
@emmebi
Copy link
Collaborator Author

emmebi commented Feb 9, 2020

Please assign this to me as I am going to do some clean-up work on this during the week.

@emmebi
Copy link
Collaborator Author

emmebi commented Feb 9, 2020

There are 45 IOUtils.closeQuietly in the code.

emmebi added a commit to emmebi/maptool that referenced this issue Feb 15, 2020
- removes the last use of copyWithClose
- also, replaces yet another try/finally with try-with-resources

Refers to RPTools#1215
emmebi added a commit to emmebi/maptool that referenced this issue Feb 15, 2020
- removes the last use of copyWithClose
- also, replaces yet another try/finally with try-with-resources

Refers to RPTools#1215
@Phergus
Copy link
Contributor

Phergus commented Feb 20, 2020

@emmebi I noticed that saved token files aren't getting their preview images in the Resource Library with the current dev code.

@emmebi
Copy link
Collaborator Author

emmebi commented Feb 21, 2020

@Phergus Can you detail this a little more? I will look into it in the week-end.

@Phergus
Copy link
Contributor

Phergus commented Feb 21, 2020

Talking about this:
Screenshot 2020-02-21 06 18 12

@emmebi
Copy link
Collaborator Author

emmebi commented Feb 21, 2020

I wonder if this could be related to #1266 instead

@emmebi
Copy link
Collaborator Author

emmebi commented Feb 21, 2020

( Drawing Foreground/Background Texture Select Using Resource Library Directory )

@Phergus
Copy link
Contributor

Phergus commented Feb 21, 2020

Maybe. It showed up very recently and I haven't had a chance to look at it more. The image is there. If I drop one of them on a map, the image shows up. Could also be #1268

@emmebi
Copy link
Collaborator Author

emmebi commented Feb 21, 2020

Ok, apparently this was introduced with this patch. I checked out the develop branch, and if you go to the "Fix input PROPS type not working", images are shown. If you move three commit forwards, this doesn't work :(

So, looking into this to understand why (I so much want to add tests ;) )

@emmebi
Copy link
Collaborator Author

emmebi commented Feb 22, 2020

Found! The problem was indeed introduced in the getFileAsInputStream, which uses again a try with resources; but in this case the return value should be the InputStream, and the InputStream should not be closed at the exit of the function.

Posting a PR for this soon.

emmebi added a commit to emmebi/maptool that referenced this issue Feb 22, 2020
getFileAsInputStream was using a try with resources, so it was always returning a closed InputStream.

Also, added a couple of unit tests as a starting point for properly testing this class.

Refers to RPTools#1215
@Phergus
Copy link
Contributor

Phergus commented Mar 3, 2020

Fix tested and working. No other issues found.

@Phergus Phergus closed this as completed Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-maintenance Adding/editing javadocs, unit tests, formatting.
Projects
None yet
Development

No branches or pull requests

2 participants