Skip to content

Commit

Permalink
[MRESOLVER-372] Rework the FileUtils collocated temp file (#365)
Browse files Browse the repository at this point in the history
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.

---

https://issues.apache.org/jira/browse/MRESOLVER-372

Backport to 1.9.x branch of the #364
  • Loading branch information
cstamas committed Nov 17, 2023
1 parent 1de8710 commit 1b8ce7f
Showing 1 changed file with 72 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,16 @@

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;

Expand All @@ -33,6 +39,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
}
Expand All @@ -52,7 +62,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;
}
Expand Down Expand Up @@ -98,23 +111,79 @@ 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)) {
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
Expand Down

0 comments on commit 1b8ce7f

Please sign in to comment.