LUCENE-8843: Only ignore IOException on dirs when invoking force (#706)

Today in the method IOUtils#fsync we ignore IOExceptions when fsyncing a
directory. However, the catch block here is too broad, for example it
would be ignoring IOExceptions when we try to open a non-existent
file. This commit addresses that by scoping the ignored exceptions only
to the invocation of FileChannel#force. This prevents us from
suppressing an exception in case we run into an unexpected issue when
opening the file.

However, 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, per the above, this
suppression also allowed other IOExceptions to be suppressed, and that
should be considered a bug (e.g., not only the directory not existing,
but any filesystem error and other reasons that we might get an access
denied there, like genuine permissions issues). 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 should
not put this burden on the caller).

lucene/core/src/java/org/apache/lucene/util/IOUtils.java

@@ -18,6 +18,7 @@ package org.apache.lucene.util;
 
 import java.io.BufferedReader;
 import java.io.Closeable;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
@@ -32,6 +33,7 @@ import java.nio.file.FileStore;
 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;
@@ -457,18 +459,28 @@ public final class IOUtils {
   public static void fsync(Path fileToSync, boolean isDir) throws IOException {
     // If the file is a directory we have to open read-only, for regular files we must open r/w for the fsync to have an effect.
     // See http://blog.httrack.com/blog/2013/11/15/everything-you-always-wanted-to-know-about-fsync/
-    try (final FileChannel file = FileChannel.open(fileToSync, isDir ? StandardOpenOption.READ : StandardOpenOption.WRITE)) {
-      file.force(true);
-    } catch (IOException ioe) {
-      if (isDir) {
-        assert (Constants.LINUX || Constants.MAC_OS_X) == false :
-            "On Linux and MacOSX fsyncing a directory should not throw IOException, "+
-                "we just don't want to rely on that in production (undocumented). Got: " + ioe;
-        // Ignore exception if it is a directory
-        return;
+    if (isDir && Constants.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 (final FileChannel file = FileChannel.open(fileToSync, isDir ? StandardOpenOption.READ : StandardOpenOption.WRITE)) {
+      try {
+        file.force(true);
+      } catch (final IOException e) {
+        if (isDir) {
+          assert (Constants.LINUX || Constants.MAC_OS_X) == false :
+              "On Linux and MacOSX fsyncing a directory should not throw IOException, " +
+                  "we just don't want to rely on that in production (undocumented). Got: " + e;
+          // Ignore exception if it is a directory
+          return;
+        }
+        // Throw original exception
+        throw e;
       }
-      // Throw original exception
-      throw ioe;
     }
   }
 

lucene/core/src/test/org/apache/lucene/util/TestIOUtils.java

@@ -19,12 +19,18 @@ package org.apache.lucene.util;
 
 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.FileStore;
 import java.nio.file.FileSystem;
 import java.nio.file.Files;
 import java.nio.file.LinkOption;
+import java.nio.file.NoSuchFileException;
+import java.nio.file.OpenOption;
 import java.nio.file.Path;
+import java.nio.file.attribute.FileAttribute;
 import java.nio.file.attribute.FileAttributeView;
 import java.nio.file.attribute.FileStoreAttributeView;
 import java.util.ArrayList;
@@ -34,7 +40,9 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Random;
+import java.util.Set;
 import java.util.UUID;
 
 import org.apache.lucene.mockfile.FilterFileSystem;
@@ -475,6 +483,43 @@ public class TestIOUtils extends LuceneTestCase {
     // 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<? extends OpenOption> 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 Exception {
     Path dir = createTempDir();
     dir = FilterPath.unwrap(dir).toRealPath();