Synchronous creation of OnDiskAttachmentStore

AttachmentStore::CreateOnDiskStore synchronously returns reference to attachment store. Internally it triggers asynchronous initialization on background thread. If initialization fails then consecutive operations with AttachmentStore fail with STORE_NOT_INITIALIZED. BUG=436285 R=maniscalco@chromium.org Review URL: https://codereview.chromium.org/757703004 Cr-Commit-Position: refs/heads/master@{#305891}
marcosholgado · Nov 26, 2014 · 39dc7ba · 39dc7ba
1 parent 0cd50eb
commit 39dc7ba
Show file tree

Hide file tree

Showing 13 changed files with 249 additions and 181 deletions.
diff --git a/components/dom_distiller/core/dom_distiller_store.cc b/components/dom_distiller/core/dom_distiller_store.cc
@@ -95,6 +95,7 @@ void DomDistillerStore::OnAttachmentsWrite(
   bool success = false;
   switch (result) {
     case syncer::AttachmentStore::UNSPECIFIED_ERROR:
+    case syncer::AttachmentStore::STORE_INITIALIZATION_FAILED:
       break;
     case syncer::AttachmentStore::SUCCESS:
       success = true;
@@ -163,6 +164,7 @@ void DomDistillerStore::OnAttachmentsRead(
   bool success = false;
   switch (result) {
     case syncer::AttachmentStore::UNSPECIFIED_ERROR:
+    case syncer::AttachmentStore::STORE_INITIALIZATION_FAILED:
       break;
     case syncer::AttachmentStore::SUCCESS:
       DCHECK(missing->empty());

diff --git a/sync/api/attachments/attachment_store.cc b/sync/api/attachments/attachment_store.cc
@@ -29,47 +29,18 @@ scoped_refptr<AttachmentStore> AttachmentStore::CreateInMemoryStore() {
       backend.Pass(), base::ThreadTaskRunnerHandle::Get()));
 }
 
-void AttachmentStore::CreateOnDiskStore(
+scoped_refptr<AttachmentStore> AttachmentStore::CreateOnDiskStore(
     const base::FilePath& path,
     const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner,
-    const CreateCallback& callback) {
-  scoped_refptr<base::SequencedTaskRunner> frontend_task_runner =
-      base::ThreadTaskRunnerHandle::Get();
-  backend_task_runner->PostTask(FROM_HERE,
-                                base::Bind(&CreateOnDiskStoreOnBackendThread,
-                                           path,
-                                           frontend_task_runner,
-                                           callback));
-}
+    const InitCallback& callback) {
+  scoped_ptr<OnDiskAttachmentStore> backend(
+      new OnDiskAttachmentStore(base::ThreadTaskRunnerHandle::Get(), path));
 
-void AttachmentStore::CreateOnDiskStoreOnBackendThread(
-    const base::FilePath& path,
-    const scoped_refptr<base::SequencedTaskRunner>& frontend_task_runner,
-    const CreateCallback& callback) {
-  scoped_ptr<OnDiskAttachmentStore> store(
-      new OnDiskAttachmentStore(frontend_task_runner));
-  Result result = store->OpenOrCreate(path);
-  if (result != SUCCESS)
-    store.reset();
-  frontend_task_runner->PostTask(FROM_HERE,
-                                 base::Bind(&CreateBackendDone,
-                                            result,
-                                            base::Passed(&store),
-                                            base::ThreadTaskRunnerHandle::Get(),
-                                            callback));
-}
+  scoped_refptr<AttachmentStore> attachment_store =
+      new AttachmentStoreHandle(backend.Pass(), backend_task_runner);
+  attachment_store->Init(callback);
 
-void AttachmentStore::CreateBackendDone(
-    const Result& result,
-    scoped_ptr<AttachmentStoreBase> backend,
-    const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner,
-    const CreateCallback& callback) {
-  scoped_refptr<AttachmentStore> store;
-  if (result == SUCCESS) {
-    store = new AttachmentStoreHandle(backend.Pass(), backend_task_runner);
-  }
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(callback, result, store));
+  return attachment_store;
 }
 
 }  // namespace syncer
diff --git a/sync/api/attachments/attachment_store.h b/sync/api/attachments/attachment_store.h
@@ -36,8 +36,10 @@ class SYNC_EXPORT AttachmentStoreBase {
   enum Result {
     SUCCESS,            // No error, all completed successfully.
     UNSPECIFIED_ERROR,  // An unspecified error occurred for one or more items.
+    STORE_INITIALIZATION_FAILED,  // AttachmentStore initialization failed.
   };
 
+  typedef base::Callback<void(const Result&)> InitCallback;
   typedef base::Callback<void(const Result&,
                               scoped_ptr<AttachmentMap>,
                               scoped_ptr<AttachmentIdList>)> ReadCallback;
@@ -50,6 +52,14 @@ class SYNC_EXPORT AttachmentStoreBase {
   AttachmentStoreBase();
   virtual ~AttachmentStoreBase();
 
+  // Asynchronously initializes attachment store.
+  //
+  // This method should not be called by consumer of this interface. It is
+  // called by factory methods in AttachmentStore class. When initialization is
+  // complete |callback| is invoked with result, in case of failure result is
+  // UNSPECIFIED_ERROR.
+  virtual void Init(const InitCallback& callback) = 0;
+
   // Asynchronously reads the attachments identified by |ids|.
   //
   // |callback| will be invoked when finished. AttachmentStore will attempt to
@@ -111,10 +121,6 @@ class SYNC_EXPORT AttachmentStore
     : public AttachmentStoreBase,
       public base::RefCountedThreadSafe<AttachmentStore> {
  public:
-  typedef base::Callback<void(const Result&,
-                              const scoped_refptr<AttachmentStore>&)>
-      CreateCallback;
-
   AttachmentStore();
 
   // Creates an AttachmentStoreHandle backed by in-memory implementation of
@@ -125,28 +131,17 @@ class SYNC_EXPORT AttachmentStore
   // attachment store. Opens corresponding leveldb database located at |path|.
   // All backend operations are scheduled to |backend_task_runner|. Opening
   // attachment store is asynchronous, once it finishes |callback| will be
-  // called on the thread that called CreateOnDiskStore. |store| parameter of
-  // callback is only valid when |result| is SUCCESS.
-  static void CreateOnDiskStore(
+  // called on the thread that called CreateOnDiskStore. Calling Read/Write/Drop
+  // before initialization completed is allowed.  Later if initialization fails
+  // these operations will fail with STORE_INITIALIZATION_FAILED error.
+  static scoped_refptr<AttachmentStore> CreateOnDiskStore(
       const base::FilePath& path,
       const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner,
-      const CreateCallback& callback);
+      const InitCallback& callback);
 
  protected:
   friend class base::RefCountedThreadSafe<AttachmentStore>;
   ~AttachmentStore() override;
-
- private:
-  static void CreateOnDiskStoreOnBackendThread(
-      const base::FilePath& path,
-      const scoped_refptr<base::SequencedTaskRunner>& frontend_task_runner,
-      const CreateCallback& callback);
-
-  static void CreateBackendDone(
-      const Result& result,
-      scoped_ptr<AttachmentStoreBase> backend,
-      const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner,
-      const CreateCallback& callback);
 };
 
 }  // namespace syncer

diff --git a/sync/internal_api/attachments/attachment_service_impl.cc b/sync/internal_api/attachments/attachment_service_impl.cc
@@ -200,7 +200,8 @@ void AttachmentServiceImpl::ReadDone(
 
   AttachmentIdList::const_iterator iter = unavailable_attachment_ids->begin();
   AttachmentIdList::const_iterator end = unavailable_attachment_ids->end();
-  if (attachment_downloader_.get()) {
+  if (result != AttachmentStore::STORE_INITIALIZATION_FAILED &&
+      attachment_downloader_.get()) {
     // Try to download locally unavailable attachments.
     for (; iter != end; ++iter) {
       attachment_downloader_->DownloadAttachment(
@@ -227,6 +228,7 @@ void AttachmentServiceImpl::WriteDone(
       state->AddAttachment(attachment);
       break;
     case AttachmentStore::UNSPECIFIED_ERROR:
+    case AttachmentStore::STORE_INITIALIZATION_FAILED:
       state->AddUnavailableAttachmentId(attachment.GetId());
       break;
   }

diff --git a/sync/internal_api/attachments/attachment_service_impl_unittest.cc b/sync/internal_api/attachments/attachment_service_impl_unittest.cc
@@ -24,6 +24,9 @@ class MockAttachmentStore : public AttachmentStore,
  public:
   MockAttachmentStore() {}
 
+  void Init(const InitCallback& callback) override {
+  }
+
   void Read(const AttachmentIdList& ids,
             const ReadCallback& callback) override {
     read_ids.push_back(ids);

diff --git a/sync/internal_api/attachments/attachment_store_handle.cc b/sync/internal_api/attachments/attachment_store_handle.cc
@@ -37,6 +37,13 @@ AttachmentStoreHandle::~AttachmentStoreHandle() {
       FROM_HERE, base::Bind(&NoOp, base::Passed(&backend_)));
 }
 
+void AttachmentStoreHandle::Init(const InitCallback& callback) {
+  DCHECK(CalledOnValidThread());
+  backend_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&AttachmentStoreBase::Init,
+                            base::Unretained(backend_.get()), callback));
+}
+
 void AttachmentStoreHandle::Read(const AttachmentIdList& ids,
                                  const ReadCallback& callback) {
   DCHECK(CalledOnValidThread());

diff --git a/sync/internal_api/attachments/attachment_store_handle_unittest.cc b/sync/internal_api/attachments/attachment_store_handle_unittest.cc
@@ -21,13 +21,15 @@ namespace {
 
 class MockAttachmentStore : public AttachmentStoreBase {
  public:
-  MockAttachmentStore(const base::Closure& read_called,
+  MockAttachmentStore(const base::Closure& init_called,
+                      const base::Closure& read_called,
                       const base::Closure& write_called,
                       const base::Closure& drop_called,
                       const base::Closure& read_metadata_called,
                       const base::Closure& read_all_metadata_called,
                       const base::Closure& dtor_called)
-      : read_called_(read_called),
+      : init_called_(init_called),
+        read_called_(read_called),
         write_called_(write_called),
         drop_called_(drop_called),
         read_metadata_called_(read_metadata_called),
@@ -36,6 +38,10 @@ class MockAttachmentStore : public AttachmentStoreBase {
 
   ~MockAttachmentStore() override { dtor_called_.Run(); }
 
+  void Init(const InitCallback& callback) override {
+    init_called_.Run();
+  }
+
   void Read(const AttachmentIdList& ids,
             const ReadCallback& callback) override {
     read_called_.Run();
@@ -60,6 +66,7 @@ class MockAttachmentStore : public AttachmentStoreBase {
     read_all_metadata_called_.Run();
   }
 
+  base::Closure init_called_;
   base::Closure read_called_;
   base::Closure write_called_;
   base::Closure drop_called_;
@@ -73,7 +80,8 @@ class MockAttachmentStore : public AttachmentStoreBase {
 class AttachmentStoreHandleTest : public testing::Test {
  protected:
   AttachmentStoreHandleTest()
-      : read_call_count_(0),
+      : init_call_count_(0),
+        read_call_count_(0),
         write_call_count_(0),
         drop_call_count_(0),
         read_metadata_call_count_(0),
@@ -82,6 +90,8 @@ class AttachmentStoreHandleTest : public testing::Test {
 
   virtual void SetUp() {
     scoped_ptr<AttachmentStoreBase> backend(new MockAttachmentStore(
+        base::Bind(&AttachmentStoreHandleTest::InitCalled,
+                   base::Unretained(this)),
         base::Bind(&AttachmentStoreHandleTest::ReadCalled,
                    base::Unretained(this)),
         base::Bind(&AttachmentStoreHandleTest::WriteCalled,
@@ -113,6 +123,8 @@ class AttachmentStoreHandleTest : public testing::Test {
     NOTREACHED();
   }
 
+  void InitCalled() { ++init_call_count_; }
+
   void ReadCalled() { ++read_call_count_; }
 
   void WriteCalled() { ++write_call_count_; }
@@ -132,6 +144,7 @@ class AttachmentStoreHandleTest : public testing::Test {
 
   base::MessageLoop message_loop_;
   scoped_refptr<AttachmentStoreHandle> attachment_store_handle_;
+  int init_call_count_;
   int read_call_count_;
   int write_call_count_;
   int drop_call_count_;
@@ -145,6 +158,12 @@ TEST_F(AttachmentStoreHandleTest, MethodsCalled) {
   AttachmentIdList ids;
   AttachmentList attachments;
 
+  attachment_store_handle_->Init(
+      base::Bind(&AttachmentStoreHandleTest::DoneWithResult));
+  EXPECT_EQ(init_call_count_, 0);
+  RunMessageLoop();
+  EXPECT_EQ(init_call_count_, 1);
+
   attachment_store_handle_->Read(
       ids, base::Bind(&AttachmentStoreHandleTest::ReadDone));
   EXPECT_EQ(read_call_count_, 0);

diff --git a/sync/internal_api/attachments/in_memory_attachment_store.cc b/sync/internal_api/attachments/in_memory_attachment_store.cc
@@ -21,6 +21,11 @@ InMemoryAttachmentStore::InMemoryAttachmentStore(
 InMemoryAttachmentStore::~InMemoryAttachmentStore() {
 }
 
+void InMemoryAttachmentStore::Init(const InitCallback& callback) {
+  DCHECK(CalledOnValidThread());
+  callback_task_runner_->PostTask(FROM_HERE, base::Bind(callback, SUCCESS));
+}
+
 void InMemoryAttachmentStore::Read(const AttachmentIdList& ids,
                                    const ReadCallback& callback) {
   DCHECK(CalledOnValidThread());