[FIX JENKINS-34855] Create a FileChannelWriter and use it for AtomicFileWriter "enforced" flushing

core/src/main/java/hudson/util/AtomicFileWriter.java

@@ -35,6 +35,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;
 
@@ -116,7 +117,7 @@ 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);
+        core = new FileChannelWriter(tmpPath, charset, false, StandardOpenOption.WRITE);
     }
 
     @Override

core/src/main/java/hudson/util/FileChannelWriter.java

@@ -0,0 +1,86 @@
+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;
+
+    /**
+     * @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, OpenOption... options) throws IOException {
+        this.charset = charset;
+        this.forceOnFlush = forceOnFlush;
+        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()) {
+            channel.force(true);
+            channel.close();
+        }
+    }
+}

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, 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));
+    }
+}

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) {