From 9a73c1d1298cf1e3c2857fe072d36503ab3f578f Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Wed, 15 Nov 2023 23:54:40 +0100 Subject: [PATCH 1/2] [MRESOLVER-372] Rework the FileUtils collocated temp file Fixes: * move() call should NOT perform the move, as writer stream to tmp file may still be open * move the file move logic to close, make it happen only when closing collocated temp file * perform fsync before atomic move to ensure there is no OS dirty buffers related to newly written file * on non-Win OS fsync the parent directory as well. --- https://issues.apache.org/jira/browse/MRESOLVER-372 --- .../org/eclipse/aether/util/FileUtils.java | 52 +++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java index dc260d768..2f0baa110 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java @@ -20,10 +20,13 @@ import java.io.Closeable; import java.io.IOException; +import java.nio.channels.FileChannel; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; +import java.nio.file.StandardOpenOption; import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.atomic.AtomicBoolean; import static java.util.Objects.requireNonNull; @@ -33,6 +36,10 @@ * @since 1.9.0 */ public final class FileUtils { + // Logic borrowed from Commons-Lang3: we really need only this, to decide do we fsync on directories or not + private static final boolean IS_WINDOWS = + System.getProperty("os.name", "unknown").startsWith("Windows"); + private FileUtils() { // hide constructor } @@ -52,7 +59,10 @@ public interface TempFile extends Closeable { */ public interface CollocatedTempFile extends TempFile { /** - * Atomically moves temp file to target file it is collocated with. + * Upon close, atomically moves temp file to target file it is collocated with overwriting target (if exists). + * Invocation of this method merely signals that caller ultimately wants temp file to replace the target + * file, but when this method returns, the move operation did not yet happen, it will happen when this + * instance is closed. */ void move() throws IOException; } @@ -98,23 +108,59 @@ public static CollocatedTempFile newTempFile(Path file) throws IOException { Path tempFile = parent.resolve(file.getFileName() + "." + Long.toUnsignedString(ThreadLocalRandom.current().nextLong()) + ".tmp"); return new CollocatedTempFile() { + private final AtomicBoolean wantsMove = new AtomicBoolean(false); + @Override public Path getPath() { return tempFile; } @Override - public void move() throws IOException { - Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE); + public void move() { + wantsMove.set(true); } @Override public void close() throws IOException { + if (wantsMove.get() && Files.isReadable(tempFile)) { + fsyncFile(tempFile); + Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE); + if (!IS_WINDOWS) { + fsyncParent(tempFile); + } + } Files.deleteIfExists(tempFile); } }; } + /** + * Performs fsync: makes sure no OS "dirty buffers" exist for given file. + * + * @param target Path that must not be {@code null}, must exist as plain file. + */ + private static void fsyncFile(Path target) throws IOException { + try (FileChannel file = FileChannel.open(target, StandardOpenOption.WRITE)) { + file.force(true); + } + } + + /** + * Performs directory fsync: not usable on Windows, but some other OSes may also throw, hence thrown IO exception + * is just ignored. + * + * @param target Path that must not be {@code null}, must exist as plain file, and must have parent. + */ + private static void fsyncParent(Path target) throws IOException { + try (FileChannel parent = FileChannel.open(target.getParent(), StandardOpenOption.READ)) { + try { + parent.force(true); + } catch (IOException e) { + // ignore + } + } + } + /** * A file writer, that accepts a {@link Path} to write some content to. Note: the file denoted by path may exist, * hence implementation have to ensure it is able to achieve its goal ("replace existing" option or equivalent From ee5b9b2a93bcc3efe06679e9d3b61ba4e6469cfc Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 16 Nov 2023 21:03:00 +0100 Subject: [PATCH 2/2] Update with new windows-copy --- .../org/eclipse/aether/util/FileUtils.java | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java index 2f0baa110..0ec3880b0 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java @@ -20,6 +20,9 @@ import java.io.Closeable; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.ByteBuffer; import java.nio.channels.FileChannel; import java.nio.file.Files; import java.nio.file.Path; @@ -123,9 +126,11 @@ public void move() { @Override public void close() throws IOException { if (wantsMove.get() && Files.isReadable(tempFile)) { - fsyncFile(tempFile); - Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE); - if (!IS_WINDOWS) { + if (IS_WINDOWS) { + copy(tempFile, file); + } else { + fsyncFile(tempFile); + Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE); fsyncParent(tempFile); } } @@ -134,6 +139,24 @@ public void close() throws IOException { }; } + /** + * On Windows we use pre-NIO2 way to copy files, as for some reason it works. Beat me why. + */ + private static void copy(Path source, Path target) throws IOException { + ByteBuffer buffer = ByteBuffer.allocate(1024 * 32); + byte[] array = buffer.array(); + try (InputStream is = Files.newInputStream(source); + OutputStream os = Files.newOutputStream(target)) { + while (true) { + int bytes = is.read(array); + if (bytes < 0) { + break; + } + os.write(array, 0, bytes); + } + } + } + /** * Performs fsync: makes sure no OS "dirty buffers" exist for given file. *