Skip to content

Commit

Permalink
Ensure PackedFile is always closed and remove PackedFile.finalize()
Browse files Browse the repository at this point in the history
Almost all `PackedFile` instances are wrapped in a try-with-resources to ensure they are always closed. The one
exception guarantees the instance is closed in a `finally` block.

The following places used to (at least partially) rely on PackedFile.finalize() but now use try-with-resources:
- `ImageFileImagePanelModel.getDecorations()`
- `PersistenceUtils.loadCampaignProperties()`
- `PackedFile`

The following place used to call `PackedFile.close()` in a `finally` block now uses try-with-resources:
- `PersistenceUtil.loadCampaign()`

The following place still uses a `finally` block because there is extra logic surrounding the close() call:
- `PersistenceUtil.saveCampaign()`
  • Loading branch information
kwvanderlinde committed Jul 31, 2023
1 parent 5811998 commit d8fb5c2
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 28 deletions.
5 changes: 0 additions & 5 deletions src/main/java/net/rptools/lib/io/PackedFile.java
Original file line number Diff line number Diff line change
Expand Up @@ -753,11 +753,6 @@ public void close() {
dirty = !file.exists();
}

@Override
protected void finalize() throws Throwable {
close();
}

protected File getExplodedFile(String path) {
return new File(tmpFile, path);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,7 @@ public Image[] getDecorations(int index) {
if (!Token.isTokenFile(fileList.get(index).getName())) {
return null;
}
try {
PackedFile pakFile = new PackedFile(fileList.get(index));
try (PackedFile pakFile = new PackedFile(fileList.get(index))) {
Object isHeroLab = pakFile.getProperty(PersistenceUtil.HERO_LAB);
if (isHeroLab != null && (boolean) isHeroLab) {
return new Image[] {herolabDecorationImage};
Expand Down
30 changes: 12 additions & 18 deletions src/main/java/net/rptools/maptool/util/PersistenceUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -435,9 +435,7 @@ public static PersistedCampaign loadCampaign(File campaignFile) throws IOExcepti
PersistedCampaign persistedCampaign = null;

// Try the new way first
PackedFile pakFile = null;
try {
pakFile = new PackedFile(campaignFile);
try (PackedFile pakFile = new PackedFile(campaignFile)) {
pakFile.setModelVersionManager(campaignVersionManager);

// Sanity check
Expand All @@ -453,7 +451,14 @@ public static PersistedCampaign loadCampaign(File campaignFile) throws IOExcepti
} catch (ConversionException ce) {
// Ignore the exception and check for "campaign == null" below...
MapTool.showError("PersistenceUtil.error.campaignVersion", ce);
} catch (ClassCastException cce) {
// Ignore the exception and check for "campaign == null" below...
MapTool.showWarning(
I18N.getText(
"PersistenceUtil.warn.campaignWrongFileType",
pakFile.getContent().getClass().getSimpleName()));
}

if (persistedCampaign != null) {
// Now load up any images that we need
// Note that the values are all placeholders
Expand All @@ -480,19 +485,12 @@ public static PersistedCampaign loadCampaign(File campaignFile) throws IOExcepti
} catch (OutOfMemoryError oom) {
MapTool.showError("Out of memory while reading campaign.", oom);
return null;
} catch (ClassCastException cce) {
MapTool.showWarning(
I18N.getText(
"PersistenceUtil.warn.campaignWrongFileType",
pakFile.getContent().getClass().getSimpleName()));
} catch (RuntimeException rte) {
MapTool.showError("PersistenceUtil.error.campaignRead", rte);
} catch (Error e) {
// Probably an issue with XStream not being able to instantiate a given class
// The old legacy technique probably won't work, but we should at least try...
MapTool.showError("PersistenceUtil.error.unknown", e);
} finally {
if (pakFile != null) pakFile.close();
}

// No longer try to load a legacy (very early 1.3 and before) campaign
Expand All @@ -511,7 +509,7 @@ private static String getThumbFilename(PackedFile pakFile) throws IOException {

public static BufferedImage getTokenThumbnail(File file) throws Exception {
BufferedImage thumb;
try (PackedFile pakFile = new PackedFile(file); ) {
try (PackedFile pakFile = new PackedFile(file)) {
// Jamz: Lets use the Large thumbnail if needed
String thumbFileName = getThumbFilename(pakFile);

Expand Down Expand Up @@ -857,9 +855,7 @@ private static boolean versionCheck(String progVersion) {
}

public static CampaignProperties loadCampaignProperties(File file) {
PackedFile pakFile = null;
try {
pakFile = new PackedFile(file);
try (PackedFile pakFile = new PackedFile(file)) {
String progVersion = (String) pakFile.getProperty(PROP_VERSION);
if (!versionCheck(progVersion)) return null;
CampaignProperties props = null;
Expand All @@ -879,10 +875,8 @@ public static CampaignProperties loadCampaignProperties(File file) {
return props;
} catch (IOException e) {
try {
if (pakFile != null)
pakFile.close(); // first close PackedFile (if it was opened) 'cuz some stupid OSes won't
// allow a file to be opened twice (ugh).
pakFile = null;
// Some OSes won't allow a file to be opened twice (ugh). But we're okay here since
// try-with-resources ensures that .close() was already called by this point.
return loadLegacyCampaignProperties(file);
} catch (IOException ioe) {
MapTool.showError("PersistenceUtil.error.campaignPropertiesLegacy", ioe);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@ public class PackedFileTest {
@Test
public void emptySave(@TempDir File tempDir) throws IOException {
File f = new File(tempDir, PACKED_TEST_FILE);
PackedFile pf = new PackedFile(f);
pf.save();
assertTrue(f.exists());
try (PackedFile pf = new PackedFile(f)) {
pf.save();
assertTrue(f.exists());
}
}

@Test
Expand Down

0 comments on commit d8fb5c2

Please sign in to comment.