This is an automated email from the ASF dual-hosted git repository.
cstamas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven-resolver.git
The following commit(s) were added to refs/heads/master by this push:
new dfdb9476 [MRESOLVER-372] Rework the FileUtils collocated temp file
(#364)
dfdb9476 is described below
commit dfdb94765637ddfb4d403b371dbc4b80b0e4bc1a
Author: Tamas Cservenak <ta...@cservenak.net>
AuthorDate: Fri Nov 17 13:26:54 2023 +0100
[MRESOLVER-372] Rework the FileUtils collocated temp file (#364)
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 windows go with old code that for some reason works (avoid NIO2)
* on non-Win OS fsync the parent directory as well.
So, to recap:
* original issue is not related to locking at all, is about change done in
1.9 where file processor was altered to use nio2 atomic move (to prevent
possibility of partial download being read by other process and prevent
incomplete files in case of crash). Maven LRM uses layout, so randomized temp
files does not matter (if they remain after crash), as they would merely just
clutter the local repository but not corrupt it.
* nuking local repo is part of healthy hygiene anyway
* seems windows (or java on windows) have issues with atomic fs ops
* resolver have no deps on p-u or any commons, api and util modules are
zero dependency jars and should remain as such
* one thing bugs me still: why does the old code work? Or in reverse, the
new why does not work? As opposed to "high frequency writes", this is not that
case (fixed by @olamy ... Um, sorry @michael-o )
To explain this last bullet: there was
https://issues.apache.org/jira/browse/MRESOLVER-325 with solution
https://github.com/apache/maven-resolver/pull/259 that "seems similar", as
originally this code was used by both, and in that case "high frequency atomic
moves" was applicable. But this now happens seemingly on "install" and "cache"
(download, place it in local repo) that has way less frequency that tracking
file write....
---
https://issues.apache.org/jira/browse/MRESOLVER-372
---
.../java/org/eclipse/aether/util/FileUtils.java | 75 +++++++++++++++++++++-
1 file changed, 72 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 dc260d76..0ec3880b 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,16 @@ package org.eclipse.aether.util;
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;
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 +39,10 @@ import static java.util.Objects.requireNonNull;
* @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 +62,10 @@ public final class FileUtils {
*/
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 +111,79 @@ public final class FileUtils {
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)) {
+ if (IS_WINDOWS) {
+ copy(tempFile, file);
+ } else {
+ fsyncFile(tempFile);
+ Files.move(tempFile, file,
StandardCopyOption.ATOMIC_MOVE);
+ fsyncParent(tempFile);
+ }
+ }
Files.deleteIfExists(tempFile);
}
};
}
+ /**
+ * 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.
+ *
+ * @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
