From d34cbd971c74e310a223891c7ca0b47b49faaa4a Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 7 Jun 2019 18:00:36 -0400 Subject: [PATCH 1/3] Fix IOUtils#fsync on Windows fsyncing directories Fsyncing directories on Windows is not possible. We always suppressed this by allowing that an AccessDeniedException is thrown when attemping to open the directory for reading. Yet, this suppression also allowed other IOExceptions to be suppressed, and that was a bug (e.g., the directory not existing, or a filesystem error and reasons that we might get an access denied there, like genuine permissions issues). This leniency was previously removed yet it exposed that we were suppressing this case on Windows. Rather than relying on exceptions for flow control and continuing to suppress there, we simply return early if attempting to fsync a directory on Windows (we will not put this burden on the caller). --- .../java/org/elasticsearch/core/internal/io/IOUtils.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java b/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java index 26395cb2c59ea..8aed4892c32d9 100644 --- a/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java +++ b/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java @@ -249,6 +249,7 @@ public FileVisitResult visitFileFailed(final Path file, final IOException exc) t } // TODO: replace with constants class if needed (cf. org.apache.lucene.util.Constants) + private static final boolean WINDOWS = System.getProperty("os.name").startsWith("Windows"); private static final boolean LINUX = System.getProperty("os.name").startsWith("Linux"); private static final boolean MAC_OS_X = System.getProperty("os.name").startsWith("Mac OS X"); @@ -263,6 +264,10 @@ public FileVisitResult visitFileFailed(final Path file, final IOException exc) t * systems and operating systems allow to fsync on a directory) */ public static void fsync(final Path fileToSync, final boolean isDir) throws IOException { + // opening a directory on Windows fails, directories can not be fsynced there + if (isDir && WINDOWS) { + return; + } try (FileChannel file = FileChannel.open(fileToSync, isDir ? StandardOpenOption.READ : StandardOpenOption.WRITE)) { try { file.force(true); From 9288dcc90bd094da4d6a7dc525a61346f300f564 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 7 Jun 2019 18:04:22 -0400 Subject: [PATCH 2/3] Move comment --- .../main/java/org/elasticsearch/core/internal/io/IOUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java b/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java index 8aed4892c32d9..2fb2c4c33da26 100644 --- a/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java +++ b/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java @@ -264,8 +264,8 @@ public FileVisitResult visitFileFailed(final Path file, final IOException exc) t * systems and operating systems allow to fsync on a directory) */ public static void fsync(final Path fileToSync, final boolean isDir) throws IOException { - // opening a directory on Windows fails, directories can not be fsynced there if (isDir && WINDOWS) { + // opening a directory on Windows fails, directories can not be fsynced there return; } try (FileChannel file = FileChannel.open(fileToSync, isDir ? StandardOpenOption.READ : StandardOpenOption.WRITE)) { From 65db9c07b6dc181558008ee8defae043d73b1244 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 7 Jun 2019 19:15:11 -0400 Subject: [PATCH 3/3] Iteration --- .../core/internal/io/IOUtils.java | 5 +++ .../core/internal/io/IOUtilsTests.java | 44 +++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java b/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java index 2fb2c4c33da26..d3e9afd4970df 100644 --- a/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java +++ b/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java @@ -24,6 +24,7 @@ import java.nio.file.FileVisitResult; import java.nio.file.FileVisitor; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.nio.file.attribute.BasicFileAttributes; @@ -266,6 +267,10 @@ public FileVisitResult visitFileFailed(final Path file, final IOException exc) t public static void fsync(final Path fileToSync, final boolean isDir) throws IOException { if (isDir && WINDOWS) { // opening a directory on Windows fails, directories can not be fsynced there + if (Files.exists(fileToSync) == false) { + // yet do not suppress trying to fsync directories that do not exist + throw new NoSuchFileException(fileToSync.toString()); + } return; } try (FileChannel file = FileChannel.open(fileToSync, isDir ? StandardOpenOption.READ : StandardOpenOption.WRITE)) { diff --git a/libs/core/src/test/java/org/elasticsearch/core/internal/io/IOUtilsTests.java b/libs/core/src/test/java/org/elasticsearch/core/internal/io/IOUtilsTests.java index ee5af323b5219..8af0f2a707e24 100644 --- a/libs/core/src/test/java/org/elasticsearch/core/internal/io/IOUtilsTests.java +++ b/libs/core/src/test/java/org/elasticsearch/core/internal/io/IOUtilsTests.java @@ -19,6 +19,7 @@ import org.apache.lucene.mockfile.FilterFileSystemProvider; import org.apache.lucene.mockfile.FilterPath; +import org.apache.lucene.util.Constants; import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.test.ESTestCase; @@ -27,14 +28,20 @@ import java.io.IOException; import java.io.OutputStream; import java.net.URI; +import java.nio.channels.FileChannel; import java.nio.charset.StandardCharsets; import java.nio.file.AccessDeniedException; import java.nio.file.FileSystem; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.OpenOption; import java.nio.file.Path; +import java.nio.file.attribute.FileAttribute; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Objects; +import java.util.Set; import java.util.function.Function; import static org.hamcrest.Matchers.arrayWithSize; @@ -214,6 +221,43 @@ public void testFsyncDirectory() throws Exception { // no exception } + private static final class AccessDeniedWhileOpeningDirectoryFileSystem extends FilterFileSystemProvider { + + AccessDeniedWhileOpeningDirectoryFileSystem(final FileSystem delegate) { + super("access_denied://", Objects.requireNonNull(delegate)); + } + + @Override + public FileChannel newFileChannel( + final Path path, + final Set options, + final FileAttribute... attrs) throws IOException { + if (Files.isDirectory(path)) { + throw new AccessDeniedException(path.toString()); + } + return delegate.newFileChannel(path, options, attrs); + } + + } + + public void testFsyncAccessDeniedOpeningDirectory() throws Exception { + final Path path = createTempDir().toRealPath(); + final FileSystem fs = new AccessDeniedWhileOpeningDirectoryFileSystem(path.getFileSystem()).getFileSystem(URI.create("file:///")); + final Path wrapped = new FilterPath(path, fs); + if (Constants.WINDOWS) { + // no exception, we early return and do not even try to open the directory + IOUtils.fsync(wrapped, true); + } else { + expectThrows(AccessDeniedException.class, () -> IOUtils.fsync(wrapped, true)); + } + } + + public void testFsyncNonExistentDirectory() throws Exception { + final Path dir = FilterPath.unwrap(createTempDir()).toRealPath(); + final Path nonExistentDir = dir.resolve("non-existent"); + expectThrows(NoSuchFileException.class, () -> IOUtils.fsync(nonExistentDir, true)); + } + public void testFsyncFile() throws IOException { final Path path = createTempDir().toRealPath(); final Path subPath = path.resolve(randomAlphaOfLength(8));