From 6976cb9bb0ddb8a9344d057969f693f975555ff5 Mon Sep 17 00:00:00 2001 From: James Seibel Date: Sat, 2 Sep 2023 07:32:54 -0500 Subject: [PATCH] Remove useless file lock from AbstractMetaDataContainerFile --- .../AbstractMetaDataContainerFile.java | 172 ++++++++---------- 1 file changed, 80 insertions(+), 92 deletions(-) diff --git a/core/src/main/java/com/seibel/distanthorizons/core/file/metaData/AbstractMetaDataContainerFile.java b/core/src/main/java/com/seibel/distanthorizons/core/file/metaData/AbstractMetaDataContainerFile.java index 31343402b..b86ac480f 100644 --- a/core/src/main/java/com/seibel/distanthorizons/core/file/metaData/AbstractMetaDataContainerFile.java +++ b/core/src/main/java/com/seibel/distanthorizons/core/file/metaData/AbstractMetaDataContainerFile.java @@ -94,7 +94,6 @@ public abstract class AbstractMetaDataContainerFile /** Should be used instead of the position inside {@link AbstractMetaDataContainerFile#baseMetaData} */ public final DhSectionPos pos; - public final ReentrantLock writeLock = new ReentrantLock(); public File file; @@ -223,106 +222,95 @@ public abstract class AbstractMetaDataContainerFile - try + File tempFile; + if (USE_ATOMIC_MOVE_REPLACE) { - // done to try preventing a rare issue where multiple threads would write/delete the same file at the same time - this.writeLock.lock(); + tempFile = new File(this.file.getPath() + ".tmp"); + //tempFile.deleteOnExit(); + } + else + { + tempFile = this.file; + } + + try (FileChannel fileChannel = FileChannel.open(tempFile.toPath(), StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)) + { + fileChannel.position(METADATA_SIZE_IN_BYTES); + + // the order of these streams is important, otherwise the checksum won't be calculated + CheckedOutputStream checkedOut = new CheckedOutputStream(Channels.newOutputStream(fileChannel), new Adler32()); + // normally a DhStream should be the topmost stream to prevent closing the stream accidentally, but since this stream will be closed immediately after writing anyway, it won't be an issue + DhDataOutputStream compressedOut = new DhDataOutputStream(checkedOut); + + // write the contained data + dataWriterFunc.writeBufferToFile(compressedOut); + compressedOut.flush(); + this.baseMetaData.checksum = (int) checkedOut.getChecksum().getValue(); - File tempFile; + // Write metadata + fileChannel.position(0); + ByteBuffer buffer = ByteBuffer.allocate(METADATA_SIZE_IN_BYTES); + buffer.putInt(METADATA_IDENTITY_BYTES); + buffer.putInt(this.pos.sectionX); + buffer.putInt(Integer.MIN_VALUE); // Unused - y pos + buffer.putInt(this.pos.sectionZ); + buffer.putInt(this.baseMetaData.checksum); + buffer.put(this.pos.sectionDetailLevel); + buffer.put(this.baseMetaData.dataLevel); + buffer.put(this.baseMetaData.binaryDataFormatVersion); + buffer.put(this.baseMetaData.worldGenStep != null ? this.baseMetaData.worldGenStep.value : EDhApiWorldGenerationStep.EMPTY.value); // TODO this null check shouldn't be necessary + buffer.putLong(this.baseMetaData.dataTypeId); + buffer.putLong(this.baseMetaData.dataVersion.get()); // for types that doesn't need data versioning, this will be Long.MAX_VALUE + LodUtil.assertTrue(buffer.remaining() == METADATA_RESERVED_SIZE); + buffer.flip(); + fileChannel.write(buffer); + + + fileChannel.close(); + //LOGGER.info("Saved file: "+this.file.getName()); + if (USE_ATOMIC_MOVE_REPLACE) { - tempFile = new File(this.file.getPath() + ".tmp"); - //tempFile.deleteOnExit(); - } - else - { - tempFile = this.file; - } - - try (FileChannel fileChannel = FileChannel.open(tempFile.toPath(), StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)) - { - fileChannel.position(METADATA_SIZE_IN_BYTES); - - // the order of these streams is important, otherwise the checksum won't be calculated - CheckedOutputStream checkedOut = new CheckedOutputStream(Channels.newOutputStream(fileChannel), new Adler32()); - // normally a DhStream should be the topmost stream to prevent closing the stream accidentally, but since this stream will be closed immediately after writing anyway, it won't be an issue - DhDataOutputStream compressedOut = new DhDataOutputStream(checkedOut); - - // write the contained data - dataWriterFunc.writeBufferToFile(compressedOut); - compressedOut.flush(); - this.baseMetaData.checksum = (int) checkedOut.getChecksum().getValue(); - - - // Write metadata - fileChannel.position(0); - ByteBuffer buffer = ByteBuffer.allocate(METADATA_SIZE_IN_BYTES); - buffer.putInt(METADATA_IDENTITY_BYTES); - buffer.putInt(this.pos.sectionX); - buffer.putInt(Integer.MIN_VALUE); // Unused - y pos - buffer.putInt(this.pos.sectionZ); - buffer.putInt(this.baseMetaData.checksum); - buffer.put(this.pos.sectionDetailLevel); - buffer.put(this.baseMetaData.dataLevel); - buffer.put(this.baseMetaData.binaryDataFormatVersion); - buffer.put(this.baseMetaData.worldGenStep != null ? this.baseMetaData.worldGenStep.value : EDhApiWorldGenerationStep.EMPTY.value); // TODO this null check shouldn't be necessary - buffer.putLong(this.baseMetaData.dataTypeId); - buffer.putLong(this.baseMetaData.dataVersion.get()); // for types that doesn't need data versioning, this will be Long.MAX_VALUE - LodUtil.assertTrue(buffer.remaining() == METADATA_RESERVED_SIZE); - buffer.flip(); - fileChannel.write(buffer); - - - fileChannel.close(); - //LOGGER.info("Saved file: "+this.file.getName()); - - if (USE_ATOMIC_MOVE_REPLACE) - { - // Atomic move / replace the actual file - Files.move(tempFile.toPath(), this.file.toPath(), StandardCopyOption.REPLACE_EXISTING); // TODO couldn't StandardCopyOption. also work here? - //LOGGER.info("replaced file: "+this.file.toPath()); - } - } - catch (NoSuchFileException e) - { - // can be thrown by the "Files.move" method if the system tries writing to an unloaded level - } - catch (ClosedChannelException e) // includes ClosedByInterruptException - { - // expected if the file handler is shut down, the exception can be ignored - //LOGGER.warn(AbstractMetaDataContainerFile.class.getSimpleName()+" file writing interrupted. Error: "+e.getMessage()); - } - finally - { - String tempDeleteErrorMessage = null; - try - { - // Delete temp file if it exists (this generally means there was an issue saving) - if (USE_ATOMIC_MOVE_REPLACE && tempFile.exists()) - { - boolean fileRemoved = tempFile.delete(); - if (!fileRemoved) - { - tempDeleteErrorMessage = "Unable to remove Temporary file at: " + tempFile.getPath(); - } - } - } - catch (SecurityException exception) - { - tempDeleteErrorMessage = "Security error: [" + exception.getMessage() + "] when attempting to remove Temporary file at: " + tempFile.getPath(); - } - - if (tempDeleteErrorMessage != null) - { - LOGGER.error(tempDeleteErrorMessage); - } - this.DebugThreadCheck = false; + // Atomic move / replace the actual file + Files.move(tempFile.toPath(), this.file.toPath(), StandardCopyOption.REPLACE_EXISTING); // TODO couldn't StandardCopyOption. also work here? + //LOGGER.info("replaced file: "+this.file.toPath()); } } + catch (NoSuchFileException e) + { + // can be thrown by the "Files.move" method if the system tries writing to an unloaded level + } + catch (ClosedChannelException e) // includes ClosedByInterruptException + { + // expected if the file handler is shut down, the exception can be ignored + //LOGGER.warn(AbstractMetaDataContainerFile.class.getSimpleName()+" file writing interrupted. Error: "+e.getMessage()); + } finally { - this.writeLock.unlock(); + String tempDeleteErrorMessage = null; + try + { + // Delete temp file if it exists (this generally means there was an issue saving) + if (USE_ATOMIC_MOVE_REPLACE && tempFile.exists()) + { + boolean fileRemoved = tempFile.delete(); + if (!fileRemoved) + { + tempDeleteErrorMessage = "Unable to remove Temporary file at: " + tempFile.getPath(); + } + } + } + catch (SecurityException exception) + { + tempDeleteErrorMessage = "Security error: [" + exception.getMessage() + "] when attempting to remove Temporary file at: " + tempFile.getPath(); + } + + if (tempDeleteErrorMessage != null) + { + LOGGER.error(tempDeleteErrorMessage); + } + this.DebugThreadCheck = false; } }