[base] POSIX File::Unlock() didn't actually unlock file.

base/files/file_unittest.cc

Index: base/files/file_unittest.cc
diff --git a/base/files/file_unittest.cc b/base/files/file_unittest.cc
index 67dbbfd1ec87eecc9eeb14f9c71d5082f2e5c847..ac754700317af69d4af5531f6c157a00a19c93db 100644
--- a/base/files/file_unittest.cc
+++ b/base/files/file_unittest.cc
@@ -2,11 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/command_line.h"
 #include "base/files/file.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/test/multiprocess_test.h"
+#include "base/test/test_timeouts.h"
+#include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "testing/multiprocess_func_list.h"
 
 using base::File;
 using base::FilePath;
@@ -509,3 +514,295 @@ TEST(FileTest, GetInfoForDirectory) {
   EXPECT_EQ(0, info.size);
 }
 #endif  // defined(OS_WIN)
+
+// Constants and helper function for lock/unlock tests.
+namespace {
+
+// Flag for the parent to share a temp dir to the child.
+const char kTempDirFlag[] = "temp-dir";
+
+// File to lock in temp dir.
+const char kLockFile[] = "lockfile";
+
+// Constants for various requests and responses, used as |signal_file| parameter
+// to signal/wait helpers.
+const char kSignalLockFileLock[] = "lock.signal";
+const char kSignalLockFileLocked[] = "locked.signal";
+const char kSignalLockFileClose[] = "close.signal";
+const char kSignalLockFileClosed[] = "closed.signal";
+const char kSignalLockFileUnlock[] = "unlock.signal";
+const char kSignalLockFileUnlocked[] = "unlocked.signal";
+const char kSignalExit[] = "exit.signal";
+
+// Signal an event by creating a file which didn't previously exist.
+bool SignalEvent(const base::FilePath& signal_dir,
+                 const base::StringPiece& signal_file) {
+  File file(signal_dir.AppendASCII(signal_file),
+            File::FLAG_CREATE | File::FLAG_APPEND);
+  return file.IsValid();
+}
+
+// Check whether an event was signaled.
+bool CheckEvent(const base::FilePath& signal_dir,
+                const base::StringPiece& signal_file) {
+  File file(signal_dir.AppendASCII(signal_file),
+            File::FLAG_OPEN | File::FLAG_READ);
+  return file.IsValid();
+}
+
+// Wait for an event to be signaled, returning false if it times out.
+bool WaitForEventWithTimeout(const base::FilePath& signal_dir,
+                             const base::StringPiece& signal_file,
+                             const base::TimeDelta& timeout) {
+  const base::Time finish_by = base::Time::Now() + timeout;
+  while (!CheckEvent(signal_dir, signal_file)) {
+    if (base::Time::Now() > finish_by)
+      return false;
+    base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(10));
+  }
+  return true;
+}
+
+// Wait forever for the event to be signaled (should never return false).
+bool WaitForEvent(const base::FilePath& signal_dir,
+                  const base::StringPiece& signal_file) {
+  return WaitForEventWithTimeout(signal_dir, signal_file,
+                                 base::TimeDelta::Max());
+}
+
+// Wait for an event or the action timeout, whichever comes first.
+bool WaitForEventOrTimeout(const base::FilePath& signal_dir,
+                           const base::StringPiece& signal_file) {
+  return WaitForEventWithTimeout(signal_dir, signal_file,
+                                 TestTimeouts::action_timeout());
+}
+
+}  // namespace
+
+// Subprocess to test getting a file lock then releasing it.
+MULTIPROCESS_TEST_MAIN(ChildLockUnlock) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  const base::FilePath temp_path =
+      command_line->GetSwitchValuePath(kTempDirFlag);
+  CHECK(base::DirectoryExists(temp_path));
+
+  // Wait for signal to lock, then lock the file.
+  CHECK(WaitForEvent(temp_path, kSignalLockFileLock));
+  File file(temp_path.AppendASCII(kLockFile),
+            File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE);
+  CHECK(file.IsValid());
+  CHECK_EQ(File::FILE_OK, file.Lock());
+  CHECK(SignalEvent(temp_path, kSignalLockFileLocked));
+
+  // Wait for signal to unlock, then unlock the file.
+  CHECK(WaitForEvent(temp_path, kSignalLockFileUnlock));
+  CHECK_EQ(File::FILE_OK, file.Unlock());
+  CHECK(SignalEvent(temp_path, kSignalLockFileUnlocked));
+
+  // Wait for signal to exit, so that the unlock can be distinguished from exit.
+  CHECK(WaitForEvent(temp_path, kSignalExit));
+  return 0;
+}
+
+// Test that the child's lock prevents the parent from getting the lock, and
+// that the child's unlock allows the parent to get the lock.
+TEST(FileTest, LockAndUnlock) {
+  // Create a temporary dir and spin up a ChildLockExit subprocess against it.
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath temp_path = temp_dir.path();
+  base::CommandLine child_command_line(
+      base::GetMultiProcessTestChildBaseCommandLine());
+  child_command_line.AppendSwitchPath(kTempDirFlag, temp_path);
+  base::Process test_child_process =

[Lei Zhang, 2015/12/04 03:23:03]

Check |test_child_process| is valid before continuing? Ditto below.

+      base::SpawnMultiProcessTestChild("ChildLockUnlock", child_command_line,
+                                       base::LaunchOptions());
+
+  // Create the lock file and verify that it can be locked and unlocked.
+  File file(temp_path.AppendASCII(kLockFile),
+            File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE);
+  ASSERT_TRUE(file.IsValid());
+  ASSERT_EQ(File::FILE_OK, file.Lock());
+  ASSERT_EQ(File::FILE_OK, file.Unlock());
+
+  // Signal child to lock the file.
+  ASSERT_TRUE(SignalEvent(temp_path, kSignalLockFileLock));
+
+  // After child has locked the file, test the lock, then signal child to unlock
+  // the file.
+  ASSERT_TRUE(WaitForEventOrTimeout(temp_path, kSignalLockFileLocked));
+  ASSERT_NE(File::FILE_OK, file.Lock());
+  ASSERT_TRUE(SignalEvent(temp_path, kSignalLockFileUnlock));
+
+  // After child has unlocked, test that the file can be locked and signal the
+  // child to exit.
+  ASSERT_TRUE(WaitForEventOrTimeout(temp_path, kSignalLockFileUnlocked));
+  ASSERT_EQ(File::FILE_OK, file.Lock());
+  ASSERT_EQ(File::FILE_OK, file.Unlock());
+  ASSERT_TRUE(SignalEvent(temp_path, kSignalExit));
+
+  int rv = -1;
+  ASSERT_TRUE(test_child_process.WaitForExitWithTimeout(
+      TestTimeouts::action_timeout(), &rv));
+  ASSERT_EQ(0, rv);
+}
+
+// Subprocess to test getting a lock then releasing it implicitly on close.
+MULTIPROCESS_TEST_MAIN(ChildLockClose) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  const base::FilePath temp_path =
+      command_line->GetSwitchValuePath(kTempDirFlag);
+  CHECK(base::DirectoryExists(temp_path));
+
+  // Wait for signal to lock, then lock the file.
+  CHECK(WaitForEvent(temp_path, kSignalLockFileLock));
+  File file(temp_path.AppendASCII(kLockFile),
+            File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE);
+  CHECK(file.IsValid());
+  CHECK_EQ(File::FILE_OK, file.Lock());
+  CHECK(SignalEvent(temp_path, kSignalLockFileLocked));
+
+  // Wait for the signal to close, then close the file.
+  CHECK(WaitForEvent(temp_path, kSignalLockFileClose));

[Lei Zhang, 2015/12/04 03:23:03]

Would it easier to have one child process impl that has a switch and can respond
to any arbitrary set of commands?

[Scott Hess - ex-Googler, 2015/12/04 06:40:34]

Just to clarify, you mean to merge the different child processes and have the
switch for unlock, unlock-by-close, and unlock-by-exit?  That would make sense.

I'm not entirely sure that the unlock-by-close and unlock-by-exit cases are
worth testing, but once one is tested the others aren't hard to add.

[Lei Zhang, 2015/12/04 16:57:54]

If it's easy to implement and it reduces the amount of code, then sure. You'll
need to be able to wait for multiple events.

If that turns out to be too complicated, then let's just stick with the current
code, but consolidate the common code at the beginning into a ChildSetup()
function.
Might as well.

+  file.Close();
+  CHECK(SignalEvent(temp_path, kSignalLockFileClosed));
+
+  // Wait for signal to exit, so that close can be distinguished from exit.
+  CHECK(WaitForEvent(temp_path, kSignalExit));
+  return 0;
+}
+
+// Test that closing the handle in the child implicitly releases the lock.
+TEST(FileTest, UnlockOnClose) {
+  // Create a temporary dir and spin up a ChildLockExit subprocess against it.
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath temp_path = temp_dir.path();
+  base::CommandLine child_command_line(
+      base::GetMultiProcessTestChildBaseCommandLine());
+  child_command_line.AppendSwitchPath(kTempDirFlag, temp_path);
+  base::Process test_child_process =
+      base::SpawnMultiProcessTestChild("ChildLockClose", child_command_line,
+                                       base::LaunchOptions());
+
+  // Create the lock file and verify that it can be locked and unlocked.
+  File file(temp_path.AppendASCII(kLockFile),
+            File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE);
+  ASSERT_TRUE(file.IsValid());
+  ASSERT_EQ(File::FILE_OK, file.Lock());
+  ASSERT_EQ(File::FILE_OK, file.Unlock());
+
+  // Signal child to lock the file.
+  ASSERT_TRUE(SignalEvent(temp_path, kSignalLockFileLock));
+
+  // After child has locked the file, test the lock, then signal child to close
+  // the file.
+  ASSERT_TRUE(WaitForEventOrTimeout(temp_path, kSignalLockFileLocked));
+  ASSERT_NE(File::FILE_OK, file.Lock());
+  ASSERT_TRUE(SignalEvent(temp_path, kSignalLockFileClose));
+
+  // After child has closed, test that the file can be locked and signal the
+  // child to exit.
+  ASSERT_TRUE(WaitForEventOrTimeout(temp_path, kSignalLockFileClosed));
+  ASSERT_EQ(File::FILE_OK, file.Lock());
+  ASSERT_EQ(File::FILE_OK, file.Unlock());
+  ASSERT_TRUE(SignalEvent(temp_path, kSignalExit));
+
+  int rv = -1;
+  ASSERT_TRUE(test_child_process.WaitForExitWithTimeout(
+      TestTimeouts::action_timeout(), &rv));
+  ASSERT_EQ(0, rv);
+}
+
+// Subprocess to test getting a lock then releasing it implicitly on exit or
+// termination.
+MULTIPROCESS_TEST_MAIN(ChildLockExit) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  const base::FilePath temp_path =
+      command_line->GetSwitchValuePath(kTempDirFlag);
+  CHECK(base::DirectoryExists(temp_path));
+
+  // Wait for signal to lock, then lock the file.
+  CHECK(WaitForEvent(temp_path, kSignalLockFileLock));
+  File file(temp_path.AppendASCII(kLockFile),
+            File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE);
+  CHECK(file.IsValid());
+  CHECK_EQ(File::FILE_OK, file.Lock());
+  CHECK(SignalEvent(temp_path, kSignalLockFileLocked));
+
+  // Wait for signal to exit, so that exit can be distinguished from kill.
+  CHECK(WaitForEvent(temp_path, kSignalExit));
+  return 0;
+}
+
+// Test that locks are released on exit.
+TEST(FileTest, UnlockOnExit) {
+  // Create a temporary dir and spin up a ChildLockExit subprocess against it.
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath temp_path = temp_dir.path();
+  base::CommandLine child_command_line(
+      base::GetMultiProcessTestChildBaseCommandLine());
+  child_command_line.AppendSwitchPath(kTempDirFlag, temp_path);
+  base::Process test_child_process =
+      base::SpawnMultiProcessTestChild("ChildLockExit", child_command_line,
+                                       base::LaunchOptions());
+
+  // Create the lock file and verify that it can be locked and unlocked.
+  File file(temp_path.AppendASCII(kLockFile),
+            File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE);
+  ASSERT_TRUE(file.IsValid());
+  ASSERT_EQ(File::FILE_OK, file.Lock());
+  ASSERT_EQ(File::FILE_OK, file.Unlock());
+
+  // Signal child to lock the file.
+  ASSERT_TRUE(SignalEvent(temp_path, kSignalLockFileLock));
+
+  // After child has locked the file, test the lock, then signal child to exit.
+  ASSERT_TRUE(WaitForEventOrTimeout(temp_path, kSignalLockFileLocked));
+  ASSERT_NE(File::FILE_OK, file.Lock());
+  ASSERT_TRUE(SignalEvent(temp_path, kSignalExit));
+
+  int rv = -1;
+  ASSERT_TRUE(test_child_process.WaitForExitWithTimeout(
+      TestTimeouts::action_timeout(), &rv));
+  ASSERT_EQ(0, rv);
+
+  // Exit released the child's lock.
+  ASSERT_EQ(File::FILE_OK, file.Lock());
+  ASSERT_EQ(File::FILE_OK, file.Unlock());
+}
+
+// Test that locks are released when a process is terminated.  This should cover
+// the case of crashing, also.
+TEST(FileTest, UnlockOnTerminate) {
+  // Create a temporary dir and spin up a ChildLockExit subprocess against it.
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath temp_path = temp_dir.path();
+  base::CommandLine child_command_line(
+      base::GetMultiProcessTestChildBaseCommandLine());
+  child_command_line.AppendSwitchPath(kTempDirFlag, temp_path);
+  base::Process test_child_process =
+      base::SpawnMultiProcessTestChild("ChildLockExit", child_command_line,
+                                       base::LaunchOptions());
+
+  // Create the lock file and verify that it can be locked and unlocked.
+  File file(temp_path.AppendASCII(kLockFile),
+            File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE);
+  ASSERT_TRUE(file.IsValid());
+  ASSERT_EQ(File::FILE_OK, file.Lock());
+  ASSERT_EQ(File::FILE_OK, file.Unlock());
+
+  // Signal child to lock the file.
+  ASSERT_TRUE(SignalEvent(temp_path, kSignalLockFileLock));
+
+  // After child has locked the file, test the lock, then terminate the child.
+  ASSERT_TRUE(WaitForEventOrTimeout(temp_path, kSignalLockFileLocked));
+  ASSERT_NE(File::FILE_OK, file.Lock());
+  ASSERT_TRUE(test_child_process.Terminate(0, true));
+
+  // Termination released the child's lock.
+  ASSERT_EQ(File::FILE_OK, file.Lock());
+  ASSERT_EQ(File::FILE_OK, file.Unlock());
+}