Merge pull request #3170 from batmat/JENKINS-34855

[FIX JENKINS-34855] Create a FileChannelWriter and use it for AtomicFileWriter "enforced" flushing
jenkinsci · Jan 12, 2018 · 438e929 · 438e929
2 parents 02d0531 + bd2cb36
commit 438e929
Show file tree

Hide file tree

Showing 4 changed files with 191 additions and 2 deletions.
diff --git a/core/src/main/java/hudson/util/AtomicFileWriter.java b/core/src/main/java/hudson/util/AtomicFileWriter.java
@@ -23,6 +23,8 @@
  */
 package hudson.util;
 
+import jenkins.util.SystemProperties;
+
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import java.io.File;
@@ -35,6 +37,7 @@
 import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
+import java.nio.file.StandardOpenOption;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -51,6 +54,15 @@ public class AtomicFileWriter extends Writer {
 
     private static final Logger LOGGER = Logger.getLogger(AtomicFileWriter.class.getName());
 
+    private static /* final */ boolean DISABLE_FORCED_FLUSH = SystemProperties.getBoolean(
+            AtomicFileWriter.class.getName() + ".DISABLE_FORCED_FLUSH");
+
+    static {
+        if (DISABLE_FORCED_FLUSH) {
+            LOGGER.log(Level.WARNING, "DISABLE_FORCED_FLUSH flag used, this could result in dataloss if failures happen in your storage subsystem.");
+        }
+    }
+
     private final Writer core;
     private final Path tmpPath;
     private final Path destPath;
@@ -93,6 +105,21 @@ private static Path toPath(@Nonnull File file) throws IOException {
      * @param charset File charset to write.
      */
     public AtomicFileWriter(@Nonnull Path destinationPath, @Nonnull Charset charset) throws IOException {
+        // See FileChannelWriter docs to understand why we do not cause a force() call on flush() from AtomicFileWriter.
+        this(destinationPath, charset, false, true);
+    }
+
+    /**
+     * <strong>DO NOT USE THIS METHOD, OR YOU WILL LOSE DATA INTEGRITY.</strong>
+     *
+     * @param destinationPath the destination path where to write the content when committed.
+     * @param charset File charset to write.
+     * @param integrityOnFlush do not force writing to disk when flushing
+     * @param integrityOnClose do not force writing to disk when closing
+     * @deprecated use {@link AtomicFileWriter#AtomicFileWriter(Path, Charset)}
+     */
+    @Deprecated
+    public AtomicFileWriter(@Nonnull Path destinationPath, @Nonnull Charset charset, boolean integrityOnFlush, boolean integrityOnClose) throws IOException {
         if (charset == null) { // be extra-defensive if people don't care
             throw new IllegalArgumentException("charset is null");
         }
@@ -116,7 +143,12 @@ public AtomicFileWriter(@Nonnull Path destinationPath, @Nonnull Charset charset)
             throw new IOException("Failed to create a temporary file in "+ dir,e);
         }
 
-        core = Files.newBufferedWriter(tmpPath, charset);
+        if (DISABLE_FORCED_FLUSH) {
+            integrityOnFlush = false;
+            integrityOnClose = false;
+        }
+
+        core = new FileChannelWriter(tmpPath, charset, integrityOnFlush, integrityOnClose, StandardOpenOption.WRITE);
     }
 
     @Override

diff --git a/core/src/main/java/hudson/util/FileChannelWriter.java b/core/src/main/java/hudson/util/FileChannelWriter.java
@@ -0,0 +1,94 @@
+package hudson.util;
+
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
+
+import java.io.IOException;
+import java.io.Writer;
+import java.nio.ByteBuffer;
+import java.nio.CharBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.charset.Charset;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.util.logging.Logger;
+
+/**
+ * This class has been created to help make {@link AtomicFileWriter} hopefully more reliable in some corner cases.
+ * We created this wrapper to be able to access {@link FileChannel#force(boolean)} which seems to be one of the rare
+ * ways to actually have a guarantee that data be flushed to the physical device (only guaranteed for local, not for
+ * remote obviously though).
+ *
+ * <p>The goal using this is to reduce as much as we can the likeliness to see zero-length files be created in place
+ * of the original ones.</p>
+ *
+ * @see <a href="https://issues.jenkins-ci.org/browse/JENKINS-34855">JENKINS-34855</a>
+ * @see <a href="https://github.com/jenkinsci/jenkins/pull/2548">PR-2548</a>
+ */
+@Restricted(NoExternalUse.class)
+public class FileChannelWriter extends Writer {
+
+    private static final Logger LOGGER = Logger.getLogger(FileChannelWriter.class.getName());
+
+    private final Charset charset;
+    private final FileChannel channel;
+
+    /**
+     * {@link FileChannel#force(boolean)} is a <strong>very</strong> costly operation. This flag has been introduced mostly to
+     * accommodate Jenkins' previous behaviour, when using a simple {@link java.io.BufferedWriter}.
+     *
+     * <p>Basically, {@link BufferedWriter#flush()} does nothing, so when existing code was rewired to use
+     * {@link FileChannelWriter#flush()} behind {@link AtomicFileWriter} and that method actually ends up calling
+     * {@link FileChannel#force(boolean)}, many things started timing out. The main reason is probably because XStream's
+     * {@link com.thoughtworks.xstream.core.util.QuickWriter} uses <code>flush()</code> a lot.
+     * So we introduced this field to be able to still get a better integrity for the use case of {@link AtomicFileWriter}.
+     * Because from there, we make sure to call {@link #close()} from {@link AtomicFileWriter#commit()} anyway.
+     */
+    private boolean forceOnFlush;
+
+    /**
+     * See forceOnFlush. You probably never want to set forceOnClose to false.
+     */
+    private boolean forceOnClose;
+
+    /**
+     * @param filePath     the path of the file to write to.
+     * @param charset      the charset to use when writing characters.
+     * @param forceOnFlush set to true if you want {@link FileChannel#force(boolean)} to be called on {@link #flush()}.
+     * @param options      the options for opening the file.
+     * @throws IOException if something went wrong.
+     */
+    FileChannelWriter(Path filePath, Charset charset, boolean forceOnFlush, boolean forceOnClose, OpenOption... options) throws IOException {
+        this.charset = charset;
+        this.forceOnFlush = forceOnFlush;
+        this.forceOnClose = forceOnClose;
+        channel = FileChannel.open(filePath, options);
+    }
+
+    @Override
+    public void write(char cbuf[], int off, int len) throws IOException {
+        final CharBuffer charBuffer = CharBuffer.wrap(cbuf, off, len);
+        ByteBuffer byteBuffer = charset.encode(charBuffer);
+        channel.write(byteBuffer);
+    }
+
+    @Override
+    public void flush() throws IOException {
+        if (forceOnFlush) {
+            LOGGER.finest("Flush is forced");
+            channel.force(true);
+        } else {
+            LOGGER.finest("Force disabled on flush(), no-op");
+        }
+    }
+
+    @Override
+    public void close() throws IOException {
+        if(channel.isOpen()) {
+            if (forceOnClose) {
+                channel.force(true);
+            }
+            channel.close();
+        }
+    }
+}
diff --git a/core/src/test/java/hudson/util/FileChannelWriterTest.java b/core/src/test/java/hudson/util/FileChannelWriterTest.java
@@ -0,0 +1,63 @@
+package hudson.util;
+
+import org.apache.commons.io.FileUtils;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.channels.ClosedChannelException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.StandardOpenOption;
+
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.core.IsEqual.equalTo;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
+
+public class FileChannelWriterTest {
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+    File file;
+    FileChannelWriter writer;
+
+    @Before
+    public void setUp() throws Exception {
+        file = temporaryFolder.newFile();
+        writer = new FileChannelWriter(file.toPath(), StandardCharsets.UTF_8, true, true,  StandardOpenOption.WRITE);
+    }
+
+    @Test
+    public void write() throws Exception {
+        writer.write("helloooo");
+        writer.close();
+
+        assertContent("helloooo");
+    }
+
+
+    @Test
+    public void flush() throws Exception {
+        writer.write("hello é è à".toCharArray());
+
+        writer.flush();
+        assertContent("hello é è à");
+    }
+
+    @Test(expected = ClosedChannelException.class)
+    public void close() throws Exception {
+        writer.write("helloooo");
+        writer.close();
+
+        writer.write("helloooo");
+        fail("Should have failed the line above");
+    }
+
+
+    private void assertContent(String string) throws IOException {
+        assertThat(FileUtils.readFileToString(file, StandardCharsets.UTF_8), equalTo(string));
+    }
+}
diff --git a/test/src/test/java/hudson/util/AtomicFileWriterPerfTest.java b/test/src/test/java/hudson/util/AtomicFileWriterPerfTest.java
@@ -24,7 +24,7 @@ public class AtomicFileWriterPerfTest {
      * <strong>really</strong> bad performance regressions.
      */
     @Issue("JENKINS-34855")
-    @Test(timeout = 30 * 1000L)
+    @Test(timeout = 50 * 1000L)
     public void poorManPerformanceTestBed() throws Exception {
         int count = 1000;
         while (count-- > 0) {