Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(18)

Issue 23658030: Revert 222154 "sync: Gracefully handle very early shutdown" (Closed)

Created:
7 years, 3 months ago by Vitaly Buka (NO REVIEWS)
Modified:
7 years, 3 months ago
Reviewers:
rlarocque
CC:
chromium-reviews
Visibility:
Public.

Description

Revert 222154 "sync: Gracefully handle very early shutdown" Makes tests very Flaky. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20Chromium&testType=sync_integration_tests&tests=TwoClientPasswordsSyncTest.SetPassphraseAndThenSetupSync Can reproduce on local build. > sync: Gracefully handle very early shutdown > > Introduce a new object to communicate cross-thread cancellation signals. > This new object, the CancellationSignal, is protected by a lock. It > allows the receiving thread to query whether or not a stop has been > requested. It also allows the receiving thread to safely register a > cross-thread callback to be invoked immediately when a stop is > requested. > > This class is used to reimplement the UI thread to sync thread early > shutdown signal. Previously, the UI thread would try to call in to > objects owned by the sync thread. This required a workaround if the > signal arrived very early, since we couldn't guarantee the sync thread > had actually created those objects until later. The CancellationSignal > is owned by the UI thread, so it is safe to call its RequestStop() at > any point during sync initialization. The sync thread will receive the > signal when it's ready. > > The new scheme has a few advantages over the old: > - Thread ownership is simpler. The SyncBackendHost::Core, SyncManager, > ServerConnectionManager, SyncScheduler and Syncer can now claim that > all their member functions run on the sync thread. > - We no longer need to implement special case logic for when a shutdown > is requested before the SyncManager has initialized. > - In a future CL, we can take advantage of the fact that we no longer > require the special case to reduce inter-thread communication during > sync startup. This will make startup simpler and, in some cases, > improve sync startup time by as much as a few hundred milliseconds. > - This will make it easier to address crbug.com/236451. > > BUG=236451 > > Review URL: https://chromiumcodereview.appspot.com/23189021 TBR=rlarocque@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222205

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+211 lines, -531 lines) Patch
M trunk/src/chrome/browser/sync/glue/sync_backend_host.h View 6 chunks +9 lines, -9 lines 0 comments Download
M trunk/src/chrome/browser/sync/glue/sync_backend_host.cc View 8 chunks +29 lines, -12 lines 0 comments Download
M trunk/src/sync/engine/net/server_connection_manager.h View 6 chunks +34 lines, -25 lines 0 comments Download
M trunk/src/sync/engine/net/server_connection_manager.cc View 5 chunks +45 lines, -32 lines 0 comments Download
M trunk/src/sync/engine/sync_scheduler.h View 1 chunk +5 lines, -3 lines 0 comments Download
M trunk/src/sync/engine/sync_scheduler_impl.h View 2 chunks +4 lines, -1 line 0 comments Download
M trunk/src/sync/engine/sync_scheduler_impl.cc View 4 chunks +13 lines, -5 lines 0 comments Download
M trunk/src/sync/engine/sync_scheduler_unittest.cc View 2 chunks +0 lines, -4 lines 0 comments Download
M trunk/src/sync/engine/syncer.h View 3 chunks +6 lines, -4 lines 0 comments Download
M trunk/src/sync/engine/syncer.cc View 2 chunks +9 lines, -4 lines 0 comments Download
M trunk/src/sync/engine/syncer_proto_util_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M trunk/src/sync/engine/syncer_unittest.cc View 3 chunks +1 line, -3 lines 0 comments Download
M trunk/src/sync/internal_api/internal_components_factory_impl.cc View 1 chunk +3 lines, -8 lines 0 comments Download
D trunk/src/sync/internal_api/public/base/cancelation_observer.h View 1 chunk +0 lines, -25 lines 0 comments Download
D trunk/src/sync/internal_api/public/base/cancelation_observer.cc View 1 chunk +0 lines, -13 lines 0 comments Download
D trunk/src/sync/internal_api/public/base/cancelation_signal.h View 1 chunk +0 lines, -72 lines 0 comments Download
D trunk/src/sync/internal_api/public/base/cancelation_signal.cc View 1 chunk +0 lines, -52 lines 0 comments Download
D trunk/src/sync/internal_api/public/base/cancelation_signal_unittest.cc View 1 chunk +0 lines, -168 lines 0 comments Download
M trunk/src/sync/internal_api/public/internal_components_factory.h View 2 chunks +1 line, -3 lines 0 comments Download
M trunk/src/sync/internal_api/public/internal_components_factory_impl.h View 1 chunk +1 line, -2 lines 0 comments Download
M trunk/src/sync/internal_api/public/sync_manager.h View 3 chunks +12 lines, -3 lines 0 comments Download
M trunk/src/sync/internal_api/public/test/fake_sync_manager.h View 2 chunks +2 lines, -2 lines 0 comments Download
M trunk/src/sync/internal_api/public/test/test_internal_components_factory.h View 1 chunk +1 line, -2 lines 0 comments Download
M trunk/src/sync/internal_api/sync_manager_impl.h View 2 chunks +2 lines, -2 lines 0 comments Download
M trunk/src/sync/internal_api/sync_manager_impl.cc View 5 chunks +10 lines, -6 lines 0 comments Download
M trunk/src/sync/internal_api/sync_manager_impl_unittest.cc View 4 chunks +2 lines, -6 lines 0 comments Download
M trunk/src/sync/internal_api/syncapi_server_connection_manager.h View 2 chunks +2 lines, -7 lines 0 comments Download
M trunk/src/sync/internal_api/syncapi_server_connection_manager.cc View 1 chunk +4 lines, -10 lines 0 comments Download
M trunk/src/sync/internal_api/syncapi_server_connection_manager_unittest.cc View 4 chunks +5 lines, -28 lines 0 comments Download
M trunk/src/sync/internal_api/test/fake_sync_manager.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M trunk/src/sync/internal_api/test/test_internal_components_factory.cc View 1 chunk +1 line, -3 lines 0 comments Download
M trunk/src/sync/sync_internal_api.gypi View 2 chunks +1 line, -5 lines 0 comments Download
M trunk/src/sync/sync_tests.gypi View 1 chunk +0 lines, -1 line 0 comments Download
M trunk/src/sync/test/engine/fake_sync_scheduler.h View 1 chunk +1 line, -1 line 0 comments Download
M trunk/src/sync/test/engine/fake_sync_scheduler.cc View 1 chunk +1 line, -1 line 0 comments Download
M trunk/src/sync/test/engine/mock_connection_manager.h View 1 chunk +0 lines, -1 line 0 comments Download
M trunk/src/sync/test/engine/mock_connection_manager.cc View 1 chunk +1 line, -1 line 0 comments Download
M trunk/src/sync/tools/sync_client.cc View 3 chunks +1 line, -4 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Vitaly Buka (NO REVIEWS)
7 years, 3 months ago (2013-09-10 08:16:50 UTC) #1
Vitaly Buka (NO REVIEWS)
Committed patchset #1 manually as r222205.
7 years, 3 months ago (2013-09-10 08:17:16 UTC) #2
rlarocque
7 years, 3 months ago (2013-09-10 18:08:02 UTC) #3
Message was sent while issue was closed.
On 2013/09/10 08:17:16, Vitaly Buka wrote:
> Committed patchset #1 manually as r222205.

LGTM, thanks.  It was definitely my patch at fault.  Sorry for the flakiness.

Powered by Google App Engine
This is Rietveld 408576698