Merge "Change AtomicFile to use rename-into-place." into rvc-dev

core/java/android/util/AtomicFile.java

@@ -29,31 +29,32 @@ import java.io.IOException;
 import java.util.function.Consumer;
 
 /**
- * Helper class for performing atomic operations on a file by creating a
+ * Helper class for performing atomic operations on a file by writing to a new file and renaming it
- * backup file until a write has successfully completed.  If you need this
+ * into the place of the original file after the write has successfully completed. If you need this
- * on older versions of the platform you can use
+ * on older versions of the platform you can use {@link androidx.core.util.AtomicFile} in AndroidX.
- * {@link android.support.v4.util.AtomicFile} in the v4 support library.
  * <p>
- * Atomic file guarantees file integrity by ensuring that a file has
+ * Atomic file guarantees file integrity by ensuring that a file has been completely written and
- * been completely written and sync'd to disk before removing its backup.
+ * sync'd to disk before renaming it to the original file. Previously this is done by renaming the
- * As long as the backup file exists, the original file is considered
+ * original file to a backup file beforehand, but this approach couldn't handle the case where the
- * to be invalid (left over from a previous attempt to write the file).
+ * file is created for the first time. This class will also handle the backup file created by the
- * </p><p>
+ * old implementation properly.
- * Atomic file does not confer any file locking semantics.
+ * <p>
- * Do not use this class when the file may be accessed or modified concurrently
+ * Atomic file does not confer any file locking semantics. Do not use this class when the file may
- * by multiple threads or processes.  The caller is responsible for ensuring
+ * be accessed or modified concurrently by multiple threads or processes. The caller is responsible
- * appropriate mutual exclusion invariants whenever it accesses the file.
+ * for ensuring appropriate mutual exclusion invariants whenever it accesses the file.
- * </p>
  */
 public class AtomicFile {
+    private static final String LOG_TAG = "AtomicFile";
+
     private final File mBaseName;
-    private final File mBackupName;
+    private final File mNewName;
+    private final File mLegacyBackupName;
     private final String mCommitTag;
     private long mStartTime;
 
     /**
      * Create a new AtomicFile for a file located at the given File path.
-     * The secondary backup file will be the same file path with ".bak" appended.
+     * The new file created when writing will be the same file path with ".new" appended.
      */
     public AtomicFile(File baseName) {
         this(baseName, null);
@@ -65,7 +66,8 @@ public class AtomicFile {
      */
     public AtomicFile(File baseName, String commitTag) {
         mBaseName = baseName;
-        mBackupName = new File(baseName.getPath() + ".bak");
+        mNewName = new File(baseName.getPath() + ".new");
+        mLegacyBackupName = new File(baseName.getPath() + ".bak");
         mCommitTag = commitTag;
     }
 
@@ -78,11 +80,12 @@ public class AtomicFile {
     }
 
     /**
-     * Delete the atomic file.  This deletes both the base and backup files.
+     * Delete the atomic file.  This deletes both the base and new files.
      */
     public void delete() {
         mBaseName.delete();
-        mBackupName.delete();
+        mNewName.delete();
+        mLegacyBackupName.delete();
     }
 
     /**
@@ -112,36 +115,28 @@ public class AtomicFile {
     public FileOutputStream startWrite(long startTime) throws IOException {
         mStartTime = startTime;
 
-        // Rename the current file so it may be used as a backup during the next read
+        if (mLegacyBackupName.exists()) {
-        if (mBaseName.exists()) {
+            if (!mLegacyBackupName.renameTo(mBaseName)) {
-            if (!mBackupName.exists()) {
+                Log.e(LOG_TAG, "Failed to rename legacy backup file " + mLegacyBackupName
-                if (!mBaseName.renameTo(mBackupName)) {
+                        + " to base file " + mBaseName);
-                    Log.w("AtomicFile", "Couldn't rename file " + mBaseName
-                            + " to backup file " + mBackupName);
-                }
-            } else {
-                mBaseName.delete();
             }
         }
-        FileOutputStream str = null;
+
         try {
-            str = new FileOutputStream(mBaseName);
+            return new FileOutputStream(mNewName);
         } catch (FileNotFoundException e) {
-            File parent = mBaseName.getParentFile();
+            File parent = mNewName.getParentFile();
             if (!parent.mkdirs()) {
-                throw new IOException("Couldn't create directory " + mBaseName);
+                throw new IOException("Failed to create directory for " + mNewName);
             }
-            FileUtils.setPermissions(
+            FileUtils.setPermissions(parent.getPath(), FileUtils.S_IRWXU | FileUtils.S_IRWXG
-                parent.getPath(),
+                    | FileUtils.S_IXOTH, -1, -1);
-                FileUtils.S_IRWXU|FileUtils.S_IRWXG|FileUtils.S_IXOTH,
-                -1, -1);
             try {
-                str = new FileOutputStream(mBaseName);
+                return new FileOutputStream(mNewName);
             } catch (FileNotFoundException e2) {
-                throw new IOException("Couldn't create " + mBaseName);
+                throw new IOException("Failed to create new file " + mNewName, e2);
             }
         }
-        return str;
     }
 
     /**
@@ -151,36 +146,45 @@ public class AtomicFile {
      * will return the new file stream.
      */
     public void finishWrite(FileOutputStream str) {
-        if (str != null) {
+        if (str == null) {
-            FileUtils.sync(str);
+            return;
-            try {
+        }
-                str.close();
+        if (!FileUtils.sync(str)) {
-                mBackupName.delete();
+            Log.e(LOG_TAG, "Failed to sync file output stream");
-            } catch (IOException e) {
+        }
-                Log.w("AtomicFile", "finishWrite: Got exception:", e);
+        try {
-            }
+            str.close();
-            if (mCommitTag != null) {
+        } catch (IOException e) {
-                com.android.internal.logging.EventLogTags.writeCommitSysConfigFile(
+            Log.e(LOG_TAG, "Failed to close file output stream", e);
-                        mCommitTag, SystemClock.uptimeMillis() - mStartTime);
+        }
-            }
+        if (!mNewName.renameTo(mBaseName)) {
+            Log.e(LOG_TAG, "Failed to rename new file " + mNewName + " to base file " + mBaseName);
+        }
+        if (mCommitTag != null) {
+            com.android.internal.logging.EventLogTags.writeCommitSysConfigFile(
+                    mCommitTag, SystemClock.uptimeMillis() - mStartTime);
         }
     }
 
     /**
      * Call when you have failed for some reason at writing to the stream
      * returned by {@link #startWrite()}.  This will close the current
-     * write stream, and roll back to the previous state of the file.
+     * write stream, and delete the new file.
      */
     public void failWrite(FileOutputStream str) {
-        if (str != null) {
+        if (str == null) {
-            FileUtils.sync(str);
+            return;
-            try {
+        }
-                str.close();
+        if (!FileUtils.sync(str)) {
-                mBaseName.delete();
+            Log.e(LOG_TAG, "Failed to sync file output stream");
-                mBackupName.renameTo(mBaseName);
+        }
-            } catch (IOException e) {
+        try {
-                Log.w("AtomicFile", "failWrite: Got exception:", e);
+            str.close();
-            }
+        } catch (IOException e) {
+            Log.e(LOG_TAG, "Failed to close file output stream", e);
+        }
+        if (!mNewName.delete()) {
+            Log.e(LOG_TAG, "Failed to delete new file " + mNewName);
         }
     }
 
@@ -210,32 +214,34 @@ public class AtomicFile {
     }
 
     /**
-     * Open the atomic file for reading.  If there previously was an
+     * Open the atomic file for reading. You should call close() on the FileInputStream when you are
-     * incomplete write, this will roll back to the last good data before
+     * done reading from it.
-     * opening for read.  You should call close() on the FileInputStream when
+     * <p>
-     * you are done reading from it.
+     * You must do your own threading protection for access to AtomicFile.
-     *
-     * <p>Note that if another thread is currently performing
-     * a write, this will incorrectly consider it to be in the state of a bad
-     * write and roll back, causing the new data currently being written to
-     * be dropped.  You must do your own threading protection for access to
-     * AtomicFile.
      */
     public FileInputStream openRead() throws FileNotFoundException {
-        if (mBackupName.exists()) {
+        if (mLegacyBackupName.exists()) {
-            mBaseName.delete();
+            if (!mLegacyBackupName.renameTo(mBaseName)) {
-            mBackupName.renameTo(mBaseName);
+                Log.e(LOG_TAG, "Failed to rename legacy backup file " + mLegacyBackupName
+                        + " to base file " + mBaseName);
+            }
+        }
+
+        if (mNewName.exists()) {
+            if (!mNewName.delete()) {
+                Log.e(LOG_TAG, "Failed to delete outdated new file " + mNewName);
+            }
         }
         return new FileInputStream(mBaseName);
     }
 
     /**
      * @hide
-     * Checks if the original or backup file exists.
+     * Checks if the original or legacy backup file exists.
-     * @return whether the original or backup file exists.
+     * @return whether the original or legacy backup file exists.
      */
     public boolean exists() {
-        return mBaseName.exists() || mBackupName.exists();
+        return mBaseName.exists() || mLegacyBackupName.exists();
     }
 
     /**
@@ -246,8 +252,8 @@ public class AtomicFile {
      *     the file does not exist or an I/O error is encountered.
      */
     public long getLastModifiedTime() {
-        if (mBackupName.exists()) {
+        if (mLegacyBackupName.exists()) {
-            return mBackupName.lastModified();
+            return mLegacyBackupName.lastModified();
         }
         return mBaseName.lastModified();
     }

core/tests/utiltests/src/android/util/AtomicFileTest.java

@@ -0,0 +1,245 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package android.util;
+
+import static org.junit.Assert.assertArrayEquals;
+
+import android.app.Instrumentation;
+import android.content.Context;
+
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
+import androidx.test.platform.app.InstrumentationRegistry;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+
+@RunWith(Parameterized.class)
+public class AtomicFileTest {
+    private static final String BASE_NAME = "base";
+    private static final String NEW_NAME = BASE_NAME + ".new";
+    private static final String LEGACY_BACKUP_NAME = BASE_NAME + ".bak";
+
+    private enum WriteAction {
+        FINISH,
+        FAIL,
+        ABORT
+    }
+
+    private static final byte[] BASE_BYTES = "base".getBytes(StandardCharsets.UTF_8);
+    private static final byte[] EXISTING_NEW_BYTES = "unnew".getBytes(StandardCharsets.UTF_8);
+    private static final byte[] NEW_BYTES = "new".getBytes(StandardCharsets.UTF_8);
+    private static final byte[] LEGACY_BACKUP_BYTES = "bak".getBytes(StandardCharsets.UTF_8);
+
+    // JUnit wants every parameter to be used so make it happy.
+    @Parameterized.Parameter()
+    public String mUnusedTestName;
+    @Nullable
+    @Parameterized.Parameter(1)
+    public String[] mExistingFileNames;
+    @Nullable
+    @Parameterized.Parameter(2)
+    public WriteAction mWriteAction;
+    @Nullable
+    @Parameterized.Parameter(3)
+    public byte[] mExpectedBytes;
+
+    private final Instrumentation mInstrumentation =
+            InstrumentationRegistry.getInstrumentation();
+    private final Context mContext = mInstrumentation.getContext();
+
+    private final File mDirectory = mContext.getFilesDir();
+    private final File mBaseFile = new File(mDirectory, BASE_NAME);
+    private final File mNewFile = new File(mDirectory, NEW_NAME);
+    private final File mLegacyBackupFile = new File(mDirectory, LEGACY_BACKUP_NAME);
+
+    @Parameterized.Parameters(name = "{0}")
+    public static Object[][] data() {
+        return new Object[][] {
+                { "none + none = none", null, null, null },
+                { "none + finish = new", null, WriteAction.FINISH, NEW_BYTES },
+                { "none + fail = none", null, WriteAction.FAIL, null },
+                { "none + abort = none", null, WriteAction.ABORT, null },
+                { "base + none = base", new String[] { BASE_NAME }, null, BASE_BYTES },
+                { "base + finish = new", new String[] { BASE_NAME }, WriteAction.FINISH,
+                        NEW_BYTES },
+                { "base + fail = base", new String[] { BASE_NAME }, WriteAction.FAIL, BASE_BYTES },
+                { "base + abort = base", new String[] { BASE_NAME }, WriteAction.ABORT,
+                        BASE_BYTES },
+                { "new + none = none", new String[] { NEW_NAME }, null, null },
+                { "new + finish = new", new String[] { NEW_NAME }, WriteAction.FINISH, NEW_BYTES },
+                { "new + fail = none", new String[] { NEW_NAME }, WriteAction.FAIL, null },
+                { "new + abort = none", new String[] { NEW_NAME }, WriteAction.ABORT, null },
+                { "bak + none = bak", new String[] { LEGACY_BACKUP_NAME }, null,
+                        LEGACY_BACKUP_BYTES },
+                { "bak + finish = new", new String[] { LEGACY_BACKUP_NAME }, WriteAction.FINISH,
+                        NEW_BYTES },
+                { "bak + fail = bak", new String[] { LEGACY_BACKUP_NAME }, WriteAction.FAIL,
+                        LEGACY_BACKUP_BYTES },
+                { "bak + abort = bak", new String[] { LEGACY_BACKUP_NAME }, WriteAction.ABORT,
+                        LEGACY_BACKUP_BYTES },
+                { "base & new + none = base", new String[] { BASE_NAME, NEW_NAME }, null,
+                        BASE_BYTES },
+                { "base & new + finish = new", new String[] { BASE_NAME, NEW_NAME },
+                        WriteAction.FINISH, NEW_BYTES },
+                { "base & new + fail = base", new String[] { BASE_NAME, NEW_NAME },
+                        WriteAction.FAIL, BASE_BYTES },
+                { "base & new + abort = base", new String[] { BASE_NAME, NEW_NAME },
+                        WriteAction.ABORT, BASE_BYTES },
+                { "base & bak + none = bak", new String[] { BASE_NAME, LEGACY_BACKUP_NAME }, null,
+                        LEGACY_BACKUP_BYTES },
+                { "base & bak + finish = new", new String[] { BASE_NAME, LEGACY_BACKUP_NAME },
+                        WriteAction.FINISH, NEW_BYTES },
+                { "base & bak + fail = bak", new String[] { BASE_NAME, LEGACY_BACKUP_NAME },
+                        WriteAction.FAIL, LEGACY_BACKUP_BYTES },
+                { "base & bak + abort = bak", new String[] { BASE_NAME, LEGACY_BACKUP_NAME },
+                        WriteAction.ABORT, LEGACY_BACKUP_BYTES },
+                { "new & bak + none = bak", new String[] { NEW_NAME, LEGACY_BACKUP_NAME }, null,
+                        LEGACY_BACKUP_BYTES },
+                { "new & bak + finish = new", new String[] { NEW_NAME, LEGACY_BACKUP_NAME },
+                        WriteAction.FINISH, NEW_BYTES },
+                { "new & bak + fail = bak", new String[] { NEW_NAME, LEGACY_BACKUP_NAME },
+                        WriteAction.FAIL, LEGACY_BACKUP_BYTES },
+                { "new & bak + abort = bak", new String[] { NEW_NAME, LEGACY_BACKUP_NAME },
+                        WriteAction.ABORT, LEGACY_BACKUP_BYTES },
+                { "base & new & bak + none = bak",
+                        new String[] { BASE_NAME, NEW_NAME, LEGACY_BACKUP_NAME }, null,
+                        LEGACY_BACKUP_BYTES },
+                { "base & new & bak + finish = new",
+                        new String[] { BASE_NAME, NEW_NAME, LEGACY_BACKUP_NAME },
+                        WriteAction.FINISH, NEW_BYTES },
+                { "base & new & bak + fail = bak",
+                        new String[] { BASE_NAME, NEW_NAME, LEGACY_BACKUP_NAME }, WriteAction.FAIL,
+                        LEGACY_BACKUP_BYTES },
+                { "base & new & bak + abort = bak",
+                        new String[] { BASE_NAME, NEW_NAME, LEGACY_BACKUP_NAME }, WriteAction.ABORT,
+                        LEGACY_BACKUP_BYTES },
+        };
+    }
+
+    @Before
+    @After
+    public void deleteFiles() {
+        mBaseFile.delete();
+        mNewFile.delete();
+        mLegacyBackupFile.delete();
+    }
+
+    @Test
+    public void testAtomicFile() throws Exception {
+        if (mExistingFileNames != null) {
+            for (String fileName : mExistingFileNames) {
+                switch (fileName) {
+                    case BASE_NAME:
+                        writeBytes(mBaseFile, BASE_BYTES);
+                        break;
+                    case NEW_NAME:
+                        writeBytes(mNewFile, EXISTING_NEW_BYTES);
+                        break;
+                    case LEGACY_BACKUP_NAME:
+                        writeBytes(mLegacyBackupFile, LEGACY_BACKUP_BYTES);
+                        break;
+                    default:
+                        throw new AssertionError(fileName);
+                }
+            }
+        }
+
+        AtomicFile atomicFile = new AtomicFile(mBaseFile);
+        if (mWriteAction != null) {
+            try (FileOutputStream outputStream = atomicFile.startWrite()) {
+                outputStream.write(NEW_BYTES);
+                switch (mWriteAction) {
+                    case FINISH:
+                        atomicFile.finishWrite(outputStream);
+                        break;
+                    case FAIL:
+                        atomicFile.failWrite(outputStream);
+                        break;
+                    case ABORT:
+                        // Neither finishing nor failing is called upon abort.
+                        break;
+                    default:
+                        throw new AssertionError(mWriteAction);
+                }
+            }
+        }
+
+        if (mExpectedBytes != null) {
+            try (FileInputStream inputStream = atomicFile.openRead()) {
+                assertArrayEquals(mExpectedBytes, readAllBytes(inputStream));
+            }
+        } else {
+            assertThrows(FileNotFoundException.class, () -> atomicFile.openRead());
+        }
+    }
+
+    private static void writeBytes(@NonNull File file, @NonNull byte[] bytes) throws IOException {
+        try (FileOutputStream outputStream = new FileOutputStream(file)) {
+            outputStream.write(bytes);
+        }
+    }
+
+    // InputStream.readAllBytes() is introduced in Java 9. Our files are small enough so that a
+    // naive implementation is okay.
+    private static byte[] readAllBytes(@NonNull InputStream inputStream) throws IOException {
+        try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) {
+            int b;
+            while ((b = inputStream.read()) != -1) {
+                outputStream.write(b);
+            }
+            return outputStream.toByteArray();
+        }
+    }
+
+    @NonNull
+    public static <T extends Throwable> T assertThrows(@NonNull Class<T> expectedType,
+            @NonNull ThrowingRunnable runnable) {
+        try {
+            runnable.run();
+        } catch (Throwable t) {
+            if (!expectedType.isInstance(t)) {
+                sneakyThrow(t);
+            }
+            //noinspection unchecked
+            return (T) t;
+        }
+        throw new AssertionError(String.format("Expected %s wasn't thrown",
+                expectedType.getSimpleName()));
+    }
+
+    private static <T extends Throwable> void sneakyThrow(@NonNull Throwable throwable) throws T {
+        //noinspection unchecked
+        throw (T) throwable;
+    }
+
+    private interface ThrowingRunnable {
+        void run() throws Throwable;
+    }
+}