From eaa7f0ec4102c4fe40145f0ebc22d8d0113672e0 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Sun, 30 Jul 2023 22:12:25 -0700 Subject: [PATCH] Ensure PackedFile is always closed and remove PackedFile.finalize() 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()` --- .../java/net/rptools/lib/io/PackedFile.java | 5 ---- .../assetpanel/ImageFileImagePanelModel.java | 3 +- .../rptools/maptool/util/PersistenceUtil.java | 30 ++++++++----------- .../net/rptools/lib/io/PackedFileTest.java | 7 +++-- 4 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/main/java/net/rptools/lib/io/PackedFile.java b/src/main/java/net/rptools/lib/io/PackedFile.java index 12604252f1..c622ad07dc 100644 --- a/src/main/java/net/rptools/lib/io/PackedFile.java +++ b/src/main/java/net/rptools/lib/io/PackedFile.java @@ -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); } diff --git a/src/main/java/net/rptools/maptool/client/ui/assetpanel/ImageFileImagePanelModel.java b/src/main/java/net/rptools/maptool/client/ui/assetpanel/ImageFileImagePanelModel.java index 239d1d7a71..ff48cbb32f 100644 --- a/src/main/java/net/rptools/maptool/client/ui/assetpanel/ImageFileImagePanelModel.java +++ b/src/main/java/net/rptools/maptool/client/ui/assetpanel/ImageFileImagePanelModel.java @@ -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}; diff --git a/src/main/java/net/rptools/maptool/util/PersistenceUtil.java b/src/main/java/net/rptools/maptool/util/PersistenceUtil.java index 7cf60dec50..012b26f2f4 100644 --- a/src/main/java/net/rptools/maptool/util/PersistenceUtil.java +++ b/src/main/java/net/rptools/maptool/util/PersistenceUtil.java @@ -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 @@ -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 @@ -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 @@ -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); @@ -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; @@ -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); diff --git a/src/test/java/net/rptools/maptool/client/swing/preference/net/rptools/lib/io/PackedFileTest.java b/src/test/java/net/rptools/maptool/client/swing/preference/net/rptools/lib/io/PackedFileTest.java index 5c89da7421..12a1d630b2 100644 --- a/src/test/java/net/rptools/maptool/client/swing/preference/net/rptools/lib/io/PackedFileTest.java +++ b/src/test/java/net/rptools/maptool/client/swing/preference/net/rptools/lib/io/PackedFileTest.java @@ -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