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

Ensure PackedFile is always closed and remove PackedFile.finalize() #4226

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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