From ef72c18b4f8de5555a04fedcb887be8152b8731b Mon Sep 17 00:00:00 2001 From: "pavely@chromium.org" Date: Mon, 2 Jun 2014 05:33:13 +0000 Subject: [PATCH] Instantiate AttachmentDownloader and use it in AttachmentServiceImpl GetOrDownloadAttachments creates state object that tracks set of attachments for which requests are still pending. Once AttachmentStore::Read finishes AttachmentServiceImpl calls AttachmentDownloader to download attachments that were not found locally. Once all calls to Downloader finish consumer callback is called with results. I used AttachmentIdSet in this code but didn't update AttachmentIdList to AttachmentIdSet in other places. I'd rather do it in separate change. R=maniscalco@chromium.org BUG=376073 Review URL: https://codereview.chromium.org/307783002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274164 0039d316-1c4b-4281-b951-d872f2087c98 --- .../profile_sync_components_factory_impl.cc | 15 +- .../generic_change_processor_unittest.cc | 3 + sync/api/attachments/attachment_id.h | 2 + .../attachments/attachment_service_impl.cc | 147 ++++++++++- .../api/attachments/attachment_service_impl.h | 12 +- .../attachment_service_impl_unittest.cc | 249 ++++++++++++++++++ sync/sync_tests.gypi | 1 + 7 files changed, 412 insertions(+), 17 deletions(-) create mode 100644 sync/api/attachments/attachment_service_impl_unittest.cc diff --git a/chrome/browser/sync/profile_sync_components_factory_impl.cc b/chrome/browser/sync/profile_sync_components_factory_impl.cc index 84708af95463..28feffe02d28 100644 --- a/chrome/browser/sync/profile_sync_components_factory_impl.cc +++ b/chrome/browser/sync/profile_sync_components_factory_impl.cc @@ -61,6 +61,7 @@ #include "sync/api/attachments/attachment_service.h" #include "sync/api/attachments/attachment_service_impl.h" #include "sync/api/syncable_service.h" +#include "sync/internal_api/public/attachments/fake_attachment_downloader.h" #include "sync/internal_api/public/attachments/fake_attachment_store.h" #include "sync/internal_api/public/attachments/fake_attachment_uploader.h" @@ -570,19 +571,23 @@ base::WeakPtr ProfileSyncComponentsFactoryImpl:: scoped_ptr ProfileSyncComponentsFactoryImpl::CreateAttachmentService( syncer::AttachmentService::Delegate* delegate) { - // TODO(maniscalco): Use a shared (one per profile) thread-safe instance of - // AttachmentUpload instead of creating a new one per AttachmentService (bug - // 369536). + // TODO(maniscalco): Use a shared (one per profile) thread-safe instances of + // AttachmentUploader and AttachmentDownloader instead of creating a new one + // per AttachmentService (bug 369536). scoped_ptr attachment_uploader( new syncer::FakeAttachmentUploader); + scoped_ptr attachment_downloader( + new syncer::FakeAttachmentDownloader()); scoped_ptr attachment_store( new syncer::FakeAttachmentStore( BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE))); scoped_ptr attachment_service( - new syncer::AttachmentServiceImpl( - attachment_store.Pass(), attachment_uploader.Pass(), delegate)); + new syncer::AttachmentServiceImpl(attachment_store.Pass(), + attachment_uploader.Pass(), + attachment_downloader.Pass(), + delegate)); return attachment_service.Pass(); } diff --git a/components/sync_driver/generic_change_processor_unittest.cc b/components/sync_driver/generic_change_processor_unittest.cc index c5259189a872..653251be4758 100644 --- a/components/sync_driver/generic_change_processor_unittest.cc +++ b/components/sync_driver/generic_change_processor_unittest.cc @@ -14,6 +14,7 @@ #include "sync/api/fake_syncable_service.h" #include "sync/api/sync_change.h" #include "sync/api/sync_merge_result.h" +#include "sync/internal_api/public/attachments/fake_attachment_downloader.h" #include "sync/internal_api/public/attachments/fake_attachment_store.h" #include "sync/internal_api/public/attachments/fake_attachment_uploader.h" #include "sync/internal_api/public/base/model_type.h" @@ -51,6 +52,8 @@ MockAttachmentService::MockAttachmentService() base::MessageLoopProxy::current())), scoped_ptr( new syncer::FakeAttachmentUploader), + scoped_ptr( + new syncer::FakeAttachmentDownloader), NULL) { } diff --git a/sync/api/attachments/attachment_id.h b/sync/api/attachments/attachment_id.h index 57a0088207d7..2971ecc1ba01 100644 --- a/sync/api/attachments/attachment_id.h +++ b/sync/api/attachments/attachment_id.h @@ -5,6 +5,7 @@ #ifndef SYNC_API_ATTACHMENTS_ATTACHMENT_ID_H_ #define SYNC_API_ATTACHMENTS_ATTACHMENT_ID_H_ +#include #include #include @@ -65,6 +66,7 @@ class SYNC_EXPORT AttachmentId { }; typedef std::vector AttachmentIdList; +typedef std::set AttachmentIdSet; } // namespace syncer diff --git a/sync/api/attachments/attachment_service_impl.cc b/sync/api/attachments/attachment_service_impl.cc index af2dea7ccc60..45134952e557 100644 --- a/sync/api/attachments/attachment_service_impl.cc +++ b/sync/api/attachments/attachment_service_impl.cc @@ -7,17 +7,113 @@ #include "base/bind.h" #include "base/message_loop/message_loop.h" #include "sync/api/attachments/attachment.h" +#include "sync/internal_api/public/attachments/fake_attachment_downloader.h" #include "sync/internal_api/public/attachments/fake_attachment_store.h" #include "sync/internal_api/public/attachments/fake_attachment_uploader.h" namespace syncer { +// GetOrDownloadAttachments starts multiple parallel DownloadAttachment calls. +// GetOrDownloadState tracks completion of these calls and posts callback for +// consumer once all attachments are either retrieved or reported unavailable. +class AttachmentServiceImpl::GetOrDownloadState + : public base::RefCounted, + public base::NonThreadSafe { + public: + // GetOrDownloadState gets parameter from values passed to + // AttachmentService::GetOrDownloadAttachments. + // |attachment_ids| is a list of attachmens to retrieve. + // |callback| will be posted on current thread when all attachments retrieved + // or confirmed unavailable. + GetOrDownloadState(const AttachmentIdList& attachment_ids, + const GetOrDownloadCallback& callback); + + // Attachment was just retrieved. Add it to retrieved attachments. + void AddAttachment(const Attachment& attachment); + + // Both reading from local store and downloading attachment failed. + // Add it to unavailable set. + void AddUnavailableAttachmentId(const AttachmentId& attachment_id); + + private: + friend class base::RefCounted; + virtual ~GetOrDownloadState(); + + // If all attachment requests completed then post callback to consumer with + // results. + void PostResultIfAllRequestsCompleted(); + + GetOrDownloadCallback callback_; + + // Requests for these attachments are still in progress. + AttachmentIdSet in_progress_attachments_; + + AttachmentIdSet unavailable_attachments_; + scoped_ptr retrieved_attachments_; + + DISALLOW_COPY_AND_ASSIGN(GetOrDownloadState); +}; + +AttachmentServiceImpl::GetOrDownloadState::GetOrDownloadState( + const AttachmentIdList& attachment_ids, + const GetOrDownloadCallback& callback) + : callback_(callback), retrieved_attachments_(new AttachmentMap()) { + std::copy( + attachment_ids.begin(), + attachment_ids.end(), + std::inserter(in_progress_attachments_, in_progress_attachments_.end())); + PostResultIfAllRequestsCompleted(); +} + +AttachmentServiceImpl::GetOrDownloadState::~GetOrDownloadState() { + DCHECK(CalledOnValidThread()); +} + +void AttachmentServiceImpl::GetOrDownloadState::AddAttachment( + const Attachment& attachment) { + DCHECK(CalledOnValidThread()); + DCHECK(retrieved_attachments_->find(attachment.GetId()) == + retrieved_attachments_->end()); + retrieved_attachments_->insert( + std::make_pair(attachment.GetId(), attachment)); + DCHECK(in_progress_attachments_.find(attachment.GetId()) != + in_progress_attachments_.end()); + in_progress_attachments_.erase(attachment.GetId()); + PostResultIfAllRequestsCompleted(); +} + +void AttachmentServiceImpl::GetOrDownloadState::AddUnavailableAttachmentId( + const AttachmentId& attachment_id) { + DCHECK(CalledOnValidThread()); + DCHECK(unavailable_attachments_.find(attachment_id) == + unavailable_attachments_.end()); + unavailable_attachments_.insert(attachment_id); + DCHECK(in_progress_attachments_.find(attachment_id) != + in_progress_attachments_.end()); + in_progress_attachments_.erase(attachment_id); + PostResultIfAllRequestsCompleted(); +} + +void +AttachmentServiceImpl::GetOrDownloadState::PostResultIfAllRequestsCompleted() { + if (in_progress_attachments_.empty()) { + // All requests completed. Let's notify consumer. + GetOrDownloadResult result = + unavailable_attachments_.empty() ? GET_SUCCESS : GET_UNSPECIFIED_ERROR; + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(callback_, result, base::Passed(&retrieved_attachments_))); + } +} + AttachmentServiceImpl::AttachmentServiceImpl( scoped_ptr attachment_store, scoped_ptr attachment_uploader, + scoped_ptr attachment_downloader, Delegate* delegate) : attachment_store_(attachment_store.Pass()), attachment_uploader_(attachment_uploader.Pass()), + attachment_downloader_(attachment_downloader.Pass()), delegate_(delegate), weak_ptr_factory_(this) { DCHECK(CalledOnValidThread()); @@ -35,9 +131,13 @@ scoped_ptr AttachmentServiceImpl::CreateForTest() { new syncer::FakeAttachmentStore(base::MessageLoopProxy::current())); scoped_ptr attachment_uploader( new FakeAttachmentUploader); + scoped_ptr attachment_downloader( + new FakeAttachmentDownloader()); scoped_ptr attachment_service( - new syncer::AttachmentServiceImpl( - attachment_store.Pass(), attachment_uploader.Pass(), NULL)); + new syncer::AttachmentServiceImpl(attachment_store.Pass(), + attachment_uploader.Pass(), + attachment_downloader.Pass(), + NULL)); return attachment_service.Pass(); } @@ -45,10 +145,12 @@ void AttachmentServiceImpl::GetOrDownloadAttachments( const AttachmentIdList& attachment_ids, const GetOrDownloadCallback& callback) { DCHECK(CalledOnValidThread()); + scoped_refptr state( + new GetOrDownloadState(attachment_ids, callback)); attachment_store_->Read(attachment_ids, base::Bind(&AttachmentServiceImpl::ReadDone, weak_ptr_factory_.GetWeakPtr(), - callback)); + state)); } void AttachmentServiceImpl::DropAttachments( @@ -96,18 +198,29 @@ void AttachmentServiceImpl::OnSyncDataUpdate( } void AttachmentServiceImpl::ReadDone( - const GetOrDownloadCallback& callback, + const scoped_refptr& state, const AttachmentStore::Result& result, scoped_ptr attachments, scoped_ptr unavailable_attachment_ids) { - AttachmentService::GetOrDownloadResult get_result = - AttachmentService::GET_UNSPECIFIED_ERROR; - if (result == AttachmentStore::SUCCESS) { - get_result = AttachmentService::GET_SUCCESS; + // Add read attachments to result. + for (AttachmentMap::const_iterator iter = attachments->begin(); + iter != attachments->end(); + ++iter) { + state->AddAttachment(iter->second); + } + // Try to download locally unavailable attachments. + for (AttachmentIdList::const_iterator iter = + unavailable_attachment_ids->begin(); + iter != unavailable_attachment_ids->end(); + ++iter) { + attachment_downloader_->DownloadAttachment( + *iter, + base::Bind(&AttachmentServiceImpl::DownloadDone, + weak_ptr_factory_.GetWeakPtr(), + state, + *iter)); + ; } - // TODO(maniscalco): Deal with case where an error occurred (bug 361251). - base::MessageLoop::current()->PostTask( - FROM_HERE, base::Bind(callback, get_result, base::Passed(&attachments))); } void AttachmentServiceImpl::DropDone(const DropCallback& callback, @@ -145,4 +258,16 @@ void AttachmentServiceImpl::UploadDone( } } +void AttachmentServiceImpl::DownloadDone( + const scoped_refptr& state, + const AttachmentId& attachment_id, + const AttachmentDownloader::DownloadResult& result, + scoped_ptr attachment) { + if (result == AttachmentDownloader::DOWNLOAD_SUCCESS) { + state->AddAttachment(*attachment.get()); + } else { + state->AddUnavailableAttachmentId(attachment_id); + } +} + } // namespace syncer diff --git a/sync/api/attachments/attachment_service_impl.h b/sync/api/attachments/attachment_service_impl.h index dbb1b530ae98..eb6d27e24559 100644 --- a/sync/api/attachments/attachment_service_impl.h +++ b/sync/api/attachments/attachment_service_impl.h @@ -5,8 +5,10 @@ #ifndef SYNC_API_ATTACHMENTS_ATTACHMENT_SERVICE_IMPL_H_ #define SYNC_API_ATTACHMENTS_ATTACHMENT_SERVICE_IMPL_H_ +#include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" #include "base/threading/non_thread_safe.h" +#include "sync/api/attachments/attachment_downloader.h" #include "sync/api/attachments/attachment_service.h" #include "sync/api/attachments/attachment_service_proxy.h" #include "sync/api/attachments/attachment_store.h" @@ -24,6 +26,7 @@ class SYNC_EXPORT AttachmentServiceImpl : public AttachmentService, // must be valid throughout AttachmentService lifetime. AttachmentServiceImpl(scoped_ptr attachment_store, scoped_ptr attachment_uploader, + scoped_ptr attachment_downloader, Delegate* delegate); virtual ~AttachmentServiceImpl(); @@ -43,7 +46,9 @@ class SYNC_EXPORT AttachmentServiceImpl : public AttachmentService, const SyncData& updated_sync_data) OVERRIDE; private: - void ReadDone(const GetOrDownloadCallback& callback, + class GetOrDownloadState; + + void ReadDone(const scoped_refptr& state, const AttachmentStore::Result& result, scoped_ptr attachments, scoped_ptr unavailable_attachment_ids); @@ -53,9 +58,14 @@ class SYNC_EXPORT AttachmentServiceImpl : public AttachmentService, const AttachmentStore::Result& result); void UploadDone(const AttachmentUploader::UploadResult& result, const AttachmentId& attachment_id); + void DownloadDone(const scoped_refptr& state, + const AttachmentId& attachment_id, + const AttachmentDownloader::DownloadResult& result, + scoped_ptr attachment); const scoped_ptr attachment_store_; const scoped_ptr attachment_uploader_; + const scoped_ptr attachment_downloader_; Delegate* delegate_; // Must be last data member. base::WeakPtrFactory weak_ptr_factory_; diff --git a/sync/api/attachments/attachment_service_impl_unittest.cc b/sync/api/attachments/attachment_service_impl_unittest.cc new file mode 100644 index 000000000000..598bd5d1863d --- /dev/null +++ b/sync/api/attachments/attachment_service_impl_unittest.cc @@ -0,0 +1,249 @@ +// Copyright 2014 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 "sync/api/attachments/attachment_service_impl.h" + +#include "base/bind.h" +#include "base/memory/weak_ptr.h" +#include "base/message_loop/message_loop.h" +#include "base/run_loop.h" +#include "sync/internal_api/public/attachments/fake_attachment_downloader.h" +#include "sync/internal_api/public/attachments/fake_attachment_uploader.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace syncer { + +class MockAttachmentStore : public AttachmentStore, + public base::SupportsWeakPtr { + public: + MockAttachmentStore() {} + + virtual void Read(const AttachmentIdList& ids, + const ReadCallback& callback) OVERRIDE { + read_ids.push_back(ids); + read_callbacks.push_back(callback); + } + + virtual void Write(const AttachmentList& attachments, + const WriteCallback& callback) OVERRIDE { + NOTREACHED(); + } + + virtual void Drop(const AttachmentIdList& ids, + const DropCallback& callback) OVERRIDE { + NOTREACHED(); + } + + // Respond to Read request. Attachments found in local_attachments should be + // returned, everything else should be reported unavailable. + void RespondToRead(const AttachmentIdSet& local_attachments) { + scoped_refptr data = new base::RefCountedString(); + ReadCallback callback = read_callbacks.back(); + AttachmentIdList ids = read_ids.back(); + read_callbacks.pop_back(); + read_ids.pop_back(); + + scoped_ptr attachments(new AttachmentMap()); + scoped_ptr unavailable_attachments( + new AttachmentIdList()); + for (AttachmentIdList::const_iterator iter = ids.begin(); iter != ids.end(); + ++iter) { + if (local_attachments.find(*iter) != local_attachments.end()) { + Attachment attachment = Attachment::CreateWithId(*iter, data); + attachments->insert(std::make_pair(*iter, attachment)); + } else { + unavailable_attachments->push_back(*iter); + } + } + Result result = + unavailable_attachments->empty() ? SUCCESS : UNSPECIFIED_ERROR; + + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(callback, + result, + base::Passed(&attachments), + base::Passed(&unavailable_attachments))); + } + + std::vector read_ids; + std::vector read_callbacks; + + DISALLOW_COPY_AND_ASSIGN(MockAttachmentStore); +}; + +class MockAttachmentDownloader + : public AttachmentDownloader, + public base::SupportsWeakPtr { + public: + MockAttachmentDownloader() {} + + virtual void DownloadAttachment(const AttachmentId& id, + const DownloadCallback& callback) OVERRIDE { + ASSERT_TRUE(download_requests.find(id) == download_requests.end()); + download_requests.insert(std::make_pair(id, callback)); + } + + // Multiple requests to download will be active at the same time. + // RespondToDownload should respond to only one of them. + void RespondToDownload(const AttachmentId& id, const DownloadResult& result) { + ASSERT_TRUE(download_requests.find(id) != download_requests.end()); + scoped_ptr attachment; + if (result == DOWNLOAD_SUCCESS) { + scoped_refptr data = new base::RefCountedString(); + attachment.reset(new Attachment(Attachment::CreateWithId(id, data))); + } + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(download_requests[id], result, base::Passed(&attachment))); + + download_requests.erase(id); + } + + std::map download_requests; + + DISALLOW_COPY_AND_ASSIGN(MockAttachmentDownloader); +}; + +class AttachmentServiceImplTest : public testing::Test { + protected: + AttachmentServiceImplTest() {} + + virtual void SetUp() OVERRIDE { + scoped_ptr attachment_store(new MockAttachmentStore()); + scoped_ptr attachment_uploader( + new FakeAttachmentUploader()); + scoped_ptr attachment_downloader( + new MockAttachmentDownloader()); + + attachment_store_ = attachment_store->AsWeakPtr(); + attachment_downloader_ = attachment_downloader->AsWeakPtr(); + + attachment_service_.reset(new AttachmentServiceImpl( + attachment_store.PassAs(), + attachment_uploader.Pass(), + attachment_downloader.PassAs(), + NULL)); + } + + virtual void TearDown() OVERRIDE { + attachment_service_.reset(); + ASSERT_FALSE(attachment_store_); + ASSERT_FALSE(attachment_downloader_); + } + + AttachmentService* attachment_service() { return attachment_service_.get(); } + + AttachmentService::GetOrDownloadCallback download_callback() { + return base::Bind(&AttachmentServiceImplTest::DownloadDone, + base::Unretained(this)); + } + + void DownloadDone(const AttachmentService::GetOrDownloadResult& result, + scoped_ptr attachments) { + download_results_.push_back(result); + last_download_attachments_ = attachments.Pass(); + } + + void RunLoop() { + base::RunLoop run_loop; + run_loop.RunUntilIdle(); + } + + const std::vector& + download_results() { + return download_results_; + } + + AttachmentMap* last_download_attachments() { + return last_download_attachments_.get(); + } + + MockAttachmentStore* store() { return attachment_store_.get(); } + + MockAttachmentDownloader* downloader() { + return attachment_downloader_.get(); + } + + private: + base::MessageLoop message_loop_; + base::WeakPtr attachment_store_; + base::WeakPtr attachment_downloader_; + scoped_ptr attachment_service_; + + std::vector download_results_; + scoped_ptr last_download_attachments_; +}; + +TEST_F(AttachmentServiceImplTest, GetOrDownload_EmptyAttachmentList) { + AttachmentIdList attachment_ids; + attachment_service()->GetOrDownloadAttachments(attachment_ids, + download_callback()); + store()->RespondToRead(AttachmentIdSet()); + + RunLoop(); + EXPECT_EQ(1U, download_results().size()); + EXPECT_EQ(0U, last_download_attachments()->size()); +} + +TEST_F(AttachmentServiceImplTest, GetOrDownload_Local) { + AttachmentIdList attachment_ids; + attachment_ids.push_back(AttachmentId::Create()); + attachment_service()->GetOrDownloadAttachments(attachment_ids, + download_callback()); + AttachmentIdSet local_attachments; + local_attachments.insert(attachment_ids[0]); + store()->RespondToRead(local_attachments); + + RunLoop(); + EXPECT_EQ(1U, download_results().size()); + EXPECT_EQ(1U, last_download_attachments()->size()); + EXPECT_TRUE(last_download_attachments()->find(attachment_ids[0]) != + last_download_attachments()->end()); +} + +TEST_F(AttachmentServiceImplTest, GetOrDownload_LocalRemoteUnavailable) { + // Create attachment list with 3 ids. + AttachmentIdList attachment_ids; + attachment_ids.push_back(AttachmentId::Create()); + attachment_ids.push_back(AttachmentId::Create()); + attachment_ids.push_back(AttachmentId::Create()); + // Call attachment service. + attachment_service()->GetOrDownloadAttachments(attachment_ids, + download_callback()); + // Ensure AttachmentStore is called. + EXPECT_FALSE(store()->read_ids.empty()); + + // make AttachmentStore return only attachment 0. + AttachmentIdSet local_attachments; + local_attachments.insert(attachment_ids[0]); + store()->RespondToRead(local_attachments); + RunLoop(); + // Ensure Downloader called with right attachment ids + EXPECT_EQ(2U, downloader()->download_requests.size()); + + // Make downloader return attachment 1. + downloader()->RespondToDownload(attachment_ids[1], + AttachmentDownloader::DOWNLOAD_SUCCESS); + RunLoop(); + // Ensure consumer callback is not called. + EXPECT_TRUE(download_results().empty()); + + // Make downloader fail attachment 2. + downloader()->RespondToDownload( + attachment_ids[2], AttachmentDownloader::DOWNLOAD_UNSPECIFIED_ERROR); + RunLoop(); + // Ensure callback is called + EXPECT_FALSE(download_results().empty()); + // There should be only two attachments returned, 0 and 1. + EXPECT_EQ(2U, last_download_attachments()->size()); + EXPECT_TRUE(last_download_attachments()->find(attachment_ids[0]) != + last_download_attachments()->end()); + EXPECT_TRUE(last_download_attachments()->find(attachment_ids[1]) != + last_download_attachments()->end()); + EXPECT_TRUE(last_download_attachments()->find(attachment_ids[2]) == + last_download_attachments()->end()); +} + +} // namespace syncer diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi index df762d8ffcab..cd4f933df035 100644 --- a/sync/sync_tests.gypi +++ b/sync/sync_tests.gypi @@ -488,6 +488,7 @@ 'sources': [ 'api/attachments/attachment_unittest.cc', 'api/attachments/attachment_id_unittest.cc', + 'api/attachments/attachment_service_impl_unittest.cc', 'api/attachments/attachment_service_proxy_unittest.cc', 'api/sync_change_unittest.cc', 'api/sync_data_unittest.cc',