Added retry logic to ConfigFileWatcher to handle sharing violation errors on Windows.

Review URL: https://chromiumcodereview.appspot.com/12288014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@182948 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 173 insertions, 14 deletions

diff --git a/remoting/host/config_file_watcher.cc b/remoting/host/config_file_watcher.cc
index 0850460..aab5fb9 100644
--- a/remoting/host/config_file_watcher.cc
+++ b/remoting/host/config_file_watcher.cc
@@ -24,6 +24,10 @@ const char kHostConfigSwitchName[] = "host-config";
 const base::FilePath::CharType kDefaultHostConfigFile[] =
     FILE_PATH_LITERAL("host.json");
 
+// Maximum number of times to try reading the configuration file before
+// reporting an error.
+const int kMaxRetries = 3;
+
 class ConfigFileWatcherImpl
     : public base::RefCountedThreadSafe<ConfigFileWatcherImpl> {
  public:
@@ -57,6 +61,9 @@ class ConfigFileWatcherImpl
 
   scoped_ptr<base::DelayTimer<ConfigFileWatcherImpl> > config_updated_timer_;
 
+  // Number of times an attempt to read the configuration file failed.
+  int retries_;
+
   // Monitors the host configuration file.
   scoped_ptr<base::FilePathWatcher> config_watcher_;
 
@@ -93,7 +100,8 @@ ConfigFileWatcherImpl::ConfigFileWatcherImpl(
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     ConfigFileWatcher::Delegate* delegate)
-    : delegate_weak_factory_(delegate),
+    : retries_(0),
+      delegate_weak_factory_(delegate),
       delegate_(delegate_weak_factory_.GetWeakPtr()),
       main_task_runner_(main_task_runner),
       io_task_runner_(io_task_runner) {
@@ -109,8 +117,8 @@ void ConfigFileWatcherImpl::Watch(const base::FilePath& config_path) {
   }
 
   DCHECK(config_path_.empty());
-  DCHECK(config_updated_timer_.get() == NULL);
-  DCHECK(config_watcher_.get() == NULL);
+  DCHECK(!config_updated_timer_);
+  DCHECK(!config_watcher_);
 
   // Create the timer that will be used for delayed-reading the configuration
   // file.
@@ -124,7 +132,7 @@ void ConfigFileWatcherImpl::Watch(const base::FilePath& config_path) {
   if (!config_watcher_->Watch(
           config_path_, false,
           base::Bind(&ConfigFileWatcherImpl::OnConfigUpdated, this))) {
-    LOG(ERROR) << "Couldn't watch file '" << config_path_.value() << "'";
+    PLOG(ERROR) << "Couldn't watch file '" << config_path_.value() << "'";
     main_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&ConfigFileWatcher::Delegate::OnConfigWatcherError,
@@ -145,15 +153,15 @@ void ConfigFileWatcherImpl::StopWatching() {
 }
 
 ConfigFileWatcherImpl::~ConfigFileWatcherImpl() {
-  DCHECK(config_updated_timer_.get() == NULL);
-  DCHECK(config_watcher_.get() == NULL);
+  DCHECK(!config_updated_timer_);
+  DCHECK(!config_watcher_);
 }
 
 void ConfigFileWatcherImpl::FinishStopping() {
   DCHECK(io_task_runner_->BelongsToCurrentThread());
 
-  config_updated_timer_.reset(NULL);
-  config_watcher_.reset(NULL);
+  config_updated_timer_.reset();
+  config_watcher_.reset();
 }
 
 void ConfigFileWatcherImpl::OnConfigUpdated(const base::FilePath& path,
@@ -173,7 +181,19 @@ void ConfigFileWatcherImpl::ReloadConfig() {
 
   std::string config;
   if (!file_util::ReadFileToString(config_path_, &config)) {
-    LOG(ERROR) << "Failed to read '" << config_path_.value() << "'";
+#if defined(OS_WIN)
+    // EACCESS may indicate a locking or sharing violation. Retry a few times
+    // before reporting an error.
+    if (errno == EACCES && retries_ < kMaxRetries) {
+      PLOG(WARNING) << "Failed to read '" << config_path_.value() << "'";
+
+      retries_ += 1;
+      config_updated_timer_->Reset();
+      return;
+    }
+#endif  // defined(OS_WIN)
+
+    PLOG(ERROR) << "Failed to read '" << config_path_.value() << "'";
     main_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&ConfigFileWatcher::Delegate::OnConfigWatcherError,
@@ -181,6 +201,8 @@ void ConfigFileWatcherImpl::ReloadConfig() {
     return;
   }
 
+  retries_ = 0;
+
   // Post an updated configuration only if it has actually changed.
   if (config_ != config) {
     config_ = config;
diff --git a/remoting/host/config_file_watcher_unittest.cc b/remoting/host/config_file_watcher_unittest.cc
new file mode 100644
index 0000000..507190c
--- /dev/null
+++ b/remoting/host/config_file_watcher_unittest.cc
@@ -0,0 +1,138 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/config_file_watcher.h"
+
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/message_loop.h"
+#include "base/run_loop.h"
+#include "remoting/base/auto_thread.h"
+#include "remoting/base/auto_thread_task_runner.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gmock_mutant.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::AnyNumber;
+using testing::Return;
+
+namespace remoting {
+
+namespace {
+
+class ConfigFileWatcherDelegate : public ConfigFileWatcher::Delegate {
+ public:
+  ConfigFileWatcherDelegate() {}
+  virtual ~ConfigFileWatcherDelegate() {}
+
+  MOCK_METHOD1(OnConfigUpdated, void(const std::string&));
+  MOCK_METHOD0(OnConfigWatcherError, void());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ConfigFileWatcherDelegate);
+};
+
+}  // namespace
+
+class ConfigFileWatcherTest : public testing::Test {
+ public:
+  ConfigFileWatcherTest();
+  virtual ~ConfigFileWatcherTest();
+
+  // testing::Test overrides
+  virtual void SetUp() OVERRIDE;
+  virtual void TearDown() OVERRIDE;
+
+  // Stops the config file watcher.
+  void StopWatcher();
+
+ protected:
+  MessageLoop message_loop_;
+  base::RunLoop run_loop_;
+
+  ConfigFileWatcherDelegate delegate_;
+
+  // Path to the configuration file used.
+  base::FilePath config_file_;
+
+  // The configuration file watcher that is being tested.
+  scoped_ptr<ConfigFileWatcher> watcher_;
+};
+
+
+ConfigFileWatcherTest::ConfigFileWatcherTest()
+    : message_loop_(MessageLoop::TYPE_UI) {
+}
+
+ConfigFileWatcherTest::~ConfigFileWatcherTest() {
+}
+
+void ConfigFileWatcherTest::StopWatcher() {
+  watcher_.reset();
+}
+
+void ConfigFileWatcherTest::SetUp() {
+  // Arrange to run |message_loop_| until no components depend on it.
+  scoped_refptr<AutoThreadTaskRunner> task_runner = new AutoThreadTaskRunner(
+      message_loop_.message_loop_proxy(), run_loop_.QuitClosure());
+
+  scoped_refptr<AutoThreadTaskRunner> io_task_runner =
+      AutoThread::CreateWithType("IPC thread", task_runner,
+                                 MessageLoop::TYPE_IO);
+
+  // Create an instance of the config watcher.
+  watcher_.reset(
+      new ConfigFileWatcher(task_runner, io_task_runner, &delegate_));
+}
+
+void ConfigFileWatcherTest::TearDown() {
+  // Delete the test file.
+  if (!config_file_.empty())
+    file_util::Delete(config_file_, false);
+}
+
+// Verifies that the initial notification is delivered.
+TEST_F(ConfigFileWatcherTest, Basic) {
+  EXPECT_TRUE(file_util::CreateTemporaryFile(&config_file_));
+
+  std::string data("test");
+  EXPECT_NE(file_util::WriteFile(config_file_, data.c_str(),
+                                 static_cast<int>(data.size())), -1);
+
+  EXPECT_CALL(delegate_, OnConfigUpdated(_))
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs(this, &ConfigFileWatcherTest::StopWatcher));
+  EXPECT_CALL(delegate_, OnConfigWatcherError())
+      .Times(0);
+
+  watcher_->Watch(config_file_);
+  run_loop_.Run();
+}
+
+MATCHER_P(EqualsString, s, "") {
+  return arg == s;
+}
+
+// Verifies that an update notification is sent when the file is changed.
+TEST_F(ConfigFileWatcherTest, Update) {
+  EXPECT_TRUE(file_util::CreateTemporaryFile(&config_file_));
+
+  EXPECT_CALL(delegate_, OnConfigUpdated(EqualsString("test")))
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs(this, &ConfigFileWatcherTest::StopWatcher));
+  EXPECT_CALL(delegate_, OnConfigWatcherError())
+      .Times(0);
+
+  watcher_->Watch(config_file_);
+
+  // Modify the watched file.
+  std::string data("test");
+  EXPECT_NE(file_util::WriteFile(config_file_, data.c_str(),
+                                 static_cast<int>(data.size())), -1);
+
+  run_loop_.Run();
+}
+
+}  // namespace remoting
diff --git a/remoting/remoting.gyp b/remoting/remoting.gyp
index 63d8ac1..091a2cf 100644
--- a/remoting/remoting.gyp
+++ b/remoting/remoting.gyp
@@ -297,6 +297,8 @@
             'host/clipboard_linux.cc',
             'host/clipboard_mac.mm',
             'host/clipboard_win.cc',
+            'host/config_file_watcher.cc',
+            'host/config_file_watcher.h',
             'host/constants_mac.cc',
             'host/constants_mac.h',
             'host/continue_window.h',
@@ -306,8 +308,8 @@
             'host/desktop_environment.h',
             'host/desktop_resizer.h',
             'host/desktop_resizer_linux.cc',
-            'host/desktop_resizer_win.cc',
             'host/desktop_resizer_mac.cc',
+            'host/desktop_resizer_win.cc',
             'host/desktop_session_connector.h',
             'host/desktop_session_proxy.cc',
             'host/desktop_session_proxy.h',
@@ -500,8 +502,6 @@
             'VERSION=<(version_full)',
           ],
           'sources': [
-            'host/config_file_watcher.cc',
-            'host/config_file_watcher.h',
             'host/curtain_mode.h',
             'host/curtaining_host_observer.h',
             'host/curtaining_host_observer.cc',
@@ -2279,8 +2279,7 @@
         'host/chromoting_host_context_unittest.cc',
         'host/chromoting_host_unittest.cc',
         'host/client_session_unittest.cc',
-        'host/config_file_watcher.cc',
-        'host/config_file_watcher.h',
+        'host/config_file_watcher_unittest.cc',
         'host/daemon_process.cc',
         'host/daemon_process.h',
         'host/daemon_process_unittest.cc',