Skip to content
Permalink
Browse files

Merge pull request #3170 from batmat/JENKINS-34855

[FIX JENKINS-34855] Create a FileChannelWriter and use it for AtomicFileWriter "enforced" flushing
  • Loading branch information
batmat committed Jan 12, 2018
2 parents 02d0531 + bd2cb36 commit 438e9296f0646d920e19e05c7696d8cec5307a88
@@ -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 @@

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
@@ -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();
}
}
}
@@ -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));
}
}
@@ -24,7 +24,7 @@
* <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) {

0 comments on commit 438e929

Please sign in to comment.
You can’t perform that action at this time.