New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 861632 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Jan 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

ModelTypeSyncBridge::MergeSyncData() should be async

Project Member Reported by mastiz@chromium.org, Jul 9

Issue description

ModelTypeSyncBridge is an asynchronous interface that USS datatypes usually need to implement. Most of the methods there are asynchronous, but a few remain synchronous.

In particular, MergeSyncData() is problematic, because it is expected that the bridge will perform a best-effort merge between local data (potentially requires I/O) and remote data (already fetched and in-memory).

Because MergeSyncData() requires access to local data, it conceptually makes little sense that it remains synchronous, while other methods like GetData() or GetAllData() are asynchronous.

In this new proposal, while a merge is ongoing, the bridge should expect no further remote updates, which the processor is responsible for. However, the local model might suffer changes, so bridges should handle this with care.

The reason why we could survive without this for so long is that many datatypes either a) keep a copy of the data in memory, or b) live in the IO thread, and can block until data is read. However, we already have a few examples where such conditions don't meet, and other workarounds are used.

One alternative API would be to instead make all methods synchronous, which would require that a number of bridges move to the IO thread.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3cfab37a6906e7d897ad16e9260cf5818323f247

commit 3cfab37a6906e7d897ad16e9260cf5818323f247
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Jul 11 22:12:30 2018

Introduce BlockingModelTypeStore[Impl]

The class is analogous to the non-blocking ModelTypeStore, which exposes
an asynchronous API.

The new class is intended to be used by datatypes that don't need to
live in the UI thread, and are instead fine to live in a background
thread, namely a sequenced task runner enforced by
BlockingModelTypeStore.

Users must use that sequenced task runner, because the underlying
(shared) LevelDB database lives there, and is not thread-safe.

The result of this refactoring introduces a minor behavioral change
which is in fact an improvement: there is now one extra hop between
threads when reading metadata, which is also the reason why some
tests need to be updated.

TBR=markusheintz@chromium.org

Bug:  861632 
Change-Id: I5db552adc42c48fc36da6378c1d7f61334f1f450
Reviewed-on: https://chromium-review.googlesource.com/1129800
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574364}
[modify] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/consent_auditor/consent_sync_bridge_impl_unittest.cc
[modify] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/BUILD.gn
[add] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model/blocking_model_type_store.h
[modify] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model/model_type_store.cc
[modify] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model/model_type_store.h
[add] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model/model_type_store_base.cc
[add] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model/model_type_store_base.h
[add] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model_impl/blocking_model_type_store_impl.cc
[add] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model_impl/blocking_model_type_store_impl.h
[modify] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model_impl/model_type_store_backend.cc
[modify] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model_impl/model_type_store_backend.h
[modify] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model_impl/model_type_store_impl.cc
[modify] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/model_impl/model_type_store_impl.h
[modify] https://crrev.com/3cfab37a6906e7d897ad16e9260cf5818323f247/components/sync/user_events/user_event_sync_bridge_unittest.cc

As per latest discussions, we will likely do the exact opposite and make ModelTypeSyncBridge's API synchronous.

In order to do that, ConsentSyncBridge and UserEventSyncBridge need to be moved to background threads.
triage ping: Mikel, can you comment on the timeline here when you come back from vacation?
sync-triage-ping with same comment as in #3 :)
As I mentioned in comment 2, I would like to do the exact opposite, but that depends on some ongoing work related to user events and consents. Meanwhile, no updates.
Status: WontFix (was: Started)
Closing this for now since plans are the exact opposite.

Sign in to add a comment