[SyncFS] Make routines using metadata database async

[SyncFS] Make routines using metadata database run asynchronously.
Some methods not used in product code keep to call GetMetadataDatabase directly.


BUG=347425
TEST=./unit_tests --gtest_filter="DriveBackendSyncTest.*:SyncEngine*:Conflict*:RegisterApp*:RemoteTo*:LocalTo*:ListChange*:MetadataDatabase*:TaskDep*"

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274213

[peria, 2014-05-29 01:21:35]

PTL

[nhiroki, 2014-05-29 01:56:13]

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:473:
MetadataDatabase* SyncEngine::GetMetadataDatabaseForTesting() {
In tests, callers of this function need to ensure that the worker is suspended
while accessing the database on the main thread. Is it right?

I guess this might cause flakiness on multiple-threads tests because future test
implementers forget the assumption. I'm not sure what the best solution is, but
at least can we have a comment about it?

[peria, 2014-05-29 02:15:29]

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:473:
MetadataDatabase* SyncEngine::GetMetadataDatabaseForTesting() {
On 2014/05/29 01:56:13, nhiroki wrote:
> In tests, callers of this function need to ensure that the worker is suspended
> while accessing the database on the main thread. Is it right?

I think it wrong.
In such a case, sync worker and DB will run independently, in racy and flaky
condition.


> I guess this might cause flakiness on multiple-threads tests because future
test
> implementers forget the assumption. I'm not sure what the best solution is,
but
> at least can we have a comment about it?

I agree.
I think it good to handle DB through methods of SyncEngine, even in tests.

[tzik, 2014-05-29 02:24:24]

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:521: void
SyncEngine::DidPromoteDemotedChanges(size_t num_trackers) {
Please merge this to OnPendingFileListUpdated

[tzik, 2014-05-29 02:31:09]

https://codereview.chromium.org/306813002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_engine.h (right):

https://codereview.chromium.org/306813002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine.h:127: // may bring
race condition.
I believe we should convert SyncEngineTest to SyncWorkerTest, which test
SyncWorker directly instead of SyncEngine.
Then, these *ForTesting function should be eventually removed.
Could you leave TODO for it?

[peria, 2014-05-29 03:11:46]

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:521: void
SyncEngine::DidPromoteDemotedChanges(size_t num_trackers) {
On 2014/05/29 02:24:25, tzik wrote:
> Please merge this to OnPendingFileListUpdated

Done.

https://codereview.chromium.org/306813002/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_engine.h (right):

https://codereview.chromium.org/306813002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine.h:127: // may bring
race condition.
On 2014/05/29 02:31:09, tzik wrote:
> I believe we should convert SyncEngineTest to SyncWorkerTest, which test
> SyncWorker directly instead of SyncEngine.
> Then, these *ForTesting function should be eventually removed.
> Could you leave TODO for it?

I filed a bug crbug.com/378621.  How about leaving this comment as-is until the
test migration is done?

[nhiroki, 2014-05-29 04:12:57]

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/306813002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:473:
MetadataDatabase* SyncEngine::GetMetadataDatabaseForTesting() {
On 2014/05/29 02:15:29, peria wrote:
> On 2014/05/29 01:56:13, nhiroki wrote:
> > In tests, callers of this function need to ensure that the worker is
suspended
> > while accessing the database on the main thread. Is it right?
> 
> I think it wrong.
> In such a case, sync worker and DB will run independently, in racy and flaky
> condition.

Sorry, "... ensure that the worker is suspended ..." is confusing. I mean to say
"... ensure that the worker isn't working on any tasks which interact with the
database ...". Does this make sense?

> 
> > I guess this might cause flakiness on multiple-threads tests because future
> test
> > implementers forget the assumption. I'm not sure what the best solution is,
> but
> > at least can we have a comment about it?
> 
> I agree.
> I think it good to handle DB through methods of SyncEngine, even in tests.

https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc
(right):

https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc:506:
return remote_sync_service_->GetMetadataDatabaseForTesting();
As chatted locally, this wrapper function easily hides the consumption I
mentioned in the previous comment. How about removing getter for metadata
database and accessing it asynchronously?

[tzik, 2014-05-30 05:23:47]

https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:514: void
SyncEngine::UpdateRegisteredAppsForTesting() {
Could you consolidate this to SyncWorker's one?

[peria, 2014-06-02 04:30:27]

PTAL.

https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc
(right):

https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc:506:
return remote_sync_service_->GetMetadataDatabaseForTesting();
On 2014/05/29 04:12:58, nhiroki wrote:
> As chatted locally, this wrapper function easily hides the consumption I
> mentioned in the previous comment. How about removing getter for metadata
> database and accessing it asynchronously?

Yes, direct calls of MetadataDatabase's methods are dangerous.
However, this getter is very useful for this test class.
I replaced all MetadataDatabase's methods with
PostTaskAndReplyWithResult and added a comment for it.
And I will add DCHECK() to check if MetadataDatabase runs on
worker task runner.

WDYT?

https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:514: void
SyncEngine::UpdateRegisteredAppsForTesting() {
On 2014/05/30 05:23:47, tzik wrote:
> Could you consolidate this to SyncWorker's one?

Done.

[tzik, 2014-06-02 05:16:40]

lgtm

[nhiroki, 2014-06-02 05:43:25]

On 2014/06/02 04:30:27, peria wrote:
> PTAL.
> 
>
https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
> File
> chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/306813002/diff/40001/chrome/browser/sync_file...
>
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc:506:
> return remote_sync_service_->GetMetadataDatabaseForTesting();
> On 2014/05/29 04:12:58, nhiroki wrote:
> > As chatted locally, this wrapper function easily hides the consumption I
> > mentioned in the previous comment. How about removing getter for metadata
> > database and accessing it asynchronously?
> 
> Yes, direct calls of MetadataDatabase's methods are dangerous.
> However, this getter is very useful for this test class.
> I replaced all MetadataDatabase's methods with
> PostTaskAndReplyWithResult and added a comment for it.
> And I will add DCHECK() to check if MetadataDatabase runs on
> worker task runner.
> 
> WDYT?

Sounds reasonable.

[nhiroki, 2014-06-02 05:43:56]

LGTM with comments.

https://codereview.chromium.org/306813002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc
(right):

https://codereview.chromium.org/306813002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc:568:
MetadataDatabase* metadata_database() {
Can you make this 'private'?

https://codereview.chromium.org/306813002/diff/140001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.h (right):

https://codereview.chromium.org/306813002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.h:146: void
UpdateRegisteredAppsForTesting();
How about having a comment like "This is not thread safe because ..."?

[peria, 2014-06-02 05:57:29]

Thank you for your reviews.

https://codereview.chromium.org/306813002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc
(right):

https://codereview.chromium.org/306813002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc:568:
MetadataDatabase* metadata_database() {
On 2014/06/02 05:43:56, nhiroki wrote:
> Can you make this 'private'?

Done.

https://codereview.chromium.org/306813002/diff/140001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.h (right):

https://codereview.chromium.org/306813002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.h:146: void
UpdateRegisteredAppsForTesting();
On 2014/06/02 05:43:56, nhiroki wrote:
> How about having a comment like "This is not thread safe because ..."?

Done.

[peria, 2014-06-02 05:57:31]

Thank you for your reviews.

[peria, 2014-06-02 05:57:41]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-06-02 05:57:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peria@chromium.org/306813002/90007

[commit-bot: I haz the power, 2014-06-02 08:05:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-02 08:06:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 08:06:25]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[peria, 2014-06-02 08:07:19]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-06-02 08:07:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peria@chromium.org/306813002/90007

[commit-bot: I haz the power, 2014-06-02 08:08:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-02 08:08:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 08:08:56]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[peria, 2014-06-02 08:10:48]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-06-02 08:11:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peria@chromium.org/306813002/90007

[commit-bot: I haz the power, 2014-06-02 08:12:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-02 08:12:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 08:12:44]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[peria, 2014-06-02 08:17:13]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-06-02 08:17:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peria@chromium.org/306813002/160001

[commit-bot: I haz the power, 2014-06-02 12:31:03]

Change committed as 274213