sync: Gracefully handle very early shutdown

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222154

[rlarocque, 2013-08-27 22:06:37]

+Fred, since he understands threading pretty well and would likely notice if
we've just re-implemented a concept that already exists somewhere else in
Chromium.

I decided to take this in a slightly different direction than I had planned to
yesterday.  Because of some C++ limitations, this code works best if the
interface (CancellationObserver) is kept minimal and we push more of the logic
into the specific implementation (ScopedConnectionHelper).

The most important files in this CL are:
- sync/internal_api/public/base/cancellation_observer.h
- sync/internal_api/public/base/cancellation_observer.cc
- sync/internal_api/public/base/cancellation_signal.h
- sync/internal_api/public/base/cancellation_signal.h
- sync/engine/net/server_connection_manager.cc
- sync/engine/net/server_connection_manager.h

[rlarocque, 2013-09-04 23:20:18]

On 2013/08/27 22:06:37, rlarocque wrote:
> +Fred, since he understands threading pretty well and would likely notice if
> we've just re-implemented a concept that already exists somewhere else in
> Chromium.
> 
> I decided to take this in a slightly different direction than I had planned to
> yesterday.  Because of some C++ limitations, this code works best if the
> interface (CancellationObserver) is kept minimal and we push more of the logic
> into the specific implementation (ScopedConnectionHelper).
> 
> The most important files in this CL are:
> - sync/internal_api/public/base/cancellation_observer.h
> - sync/internal_api/public/base/cancellation_observer.cc
> - sync/internal_api/public/base/cancellation_signal.h
> - sync/internal_api/public/base/cancellation_signal.h
> - sync/engine/net/server_connection_manager.cc
> - sync/engine/net/server_connection_manager.h

ping.

[akalin, 2013-09-05 15:35:27]

still looking, will finish by today

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.cc (right):

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:141: if (connection_) {
i don't think this clause is needed

[akalin, 2013-09-05 16:06:35]

threading lgtm, I don't think there's a general-purpose thing that can do what
CancellableSignal does.

I'll leave the overall review to Tim.

https://codereview.chromium.org/23189021/diff/13001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.h (right):

https://codereview.chromium.org/23189021/diff/13001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:367: syncer::CancellationSignal*
cancellation_signal;
const?

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.cc (right):

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:278: ScopedConnectionHelper
post(cancellation_signal_, MakeConnection());
ideally, MakeConnection() would return a scoped_ptr<> too

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.h (right):

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:307: CancellationSignal*
cancellation_signal_;
const?

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:318: // Takes ownership of
|connection|.
make this take scoped_ptr<Connection> instead of the comment?

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:329: virtual void OnStopRequested()
OVERRIDE FINAL;
newline before private:

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:331: CancellationSignal*
cancellation_signal_;
const?

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:333:
DISALLOW_COPY_AND_ASSIGN(ScopedConnectionHelper);
i believe there should be a newline before DISALLOW

https://codereview.chromium.org/23189021/diff/13001/sync/engine/syncer.h
File sync/engine/syncer.h (right):

https://codereview.chromium.org/23189021/diff/13001/sync/engine/syncer.h#newc...
sync/engine/syncer.h:81: syncer::CancellationSignal* cancellation_signal_;
const?

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
File sync/internal_api/public/base/cancellation_observer.h (right):

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
sync/internal_api/public/base/cancellation_observer.h:5: #ifndef
SYNC_INTERNAL_API_PUBLIC_BASE_CANCELLATION_OBSERVER_H_
worth pointing out that cancelable etc. seems to be more common in chromium than
cancellable, etc. (I don't care either way.)

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
File sync/internal_api/public/base/cancellation_signal.cc (right):

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
sync/internal_api/public/base/cancellation_signal.cc:14: handler_(NULL){ }
space after )

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
File sync/internal_api/public/base/cancellation_signal.h (right):

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
sync/internal_api/public/base/cancellation_signal.h:55: // registered handler,
if one exists.
might want to mention that OnStopRequested() is called under lock.

[rlarocque, 2013-09-05 22:37:06]

Patch updated.

https://codereview.chromium.org/23189021/diff/13001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.h (right):

https://codereview.chromium.org/23189021/diff/13001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:367: syncer::CancellationSignal*
cancellation_signal;
On 2013/09/05 16:06:35, akalin wrote:
> const?

Done.

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.cc (right):

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:141: if (connection_) {
On 2013/09/05 15:35:28, akalin wrote:
> i don't think this clause is needed

You're right.  Fixed.

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:278: ScopedConnectionHelper
post(cancellation_signal_, MakeConnection());
On 2013/09/05 16:06:35, akalin wrote:
> ideally, MakeConnection() would return a scoped_ptr<> too

Done.

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.h (right):

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:307: CancellationSignal*
cancellation_signal_;
On 2013/09/05 16:06:35, akalin wrote:
> const?

Done.

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:318: // Takes ownership of
|connection|.
On 2013/09/05 16:06:35, akalin wrote:
> make this take scoped_ptr<Connection> instead of the comment?

Done.

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:329: virtual void OnStopRequested()
OVERRIDE FINAL;
On 2013/09/05 16:06:35, akalin wrote:
> newline before private:

Done.

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:331: CancellationSignal*
cancellation_signal_;
On 2013/09/05 16:06:35, akalin wrote:
> const?

Done.

https://codereview.chromium.org/23189021/diff/13001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:333:
DISALLOW_COPY_AND_ASSIGN(ScopedConnectionHelper);
On 2013/09/05 16:06:35, akalin wrote:
> i believe there should be a newline before DISALLOW

Done.

https://codereview.chromium.org/23189021/diff/13001/sync/engine/syncer.h
File sync/engine/syncer.h (right):

https://codereview.chromium.org/23189021/diff/13001/sync/engine/syncer.h#newc...
sync/engine/syncer.h:81: syncer::CancellationSignal* cancellation_signal_;
On 2013/09/05 16:06:35, akalin wrote:
> const?

Done.

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
File sync/internal_api/public/base/cancellation_observer.h (right):

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
sync/internal_api/public/base/cancellation_observer.h:5: #ifndef
SYNC_INTERNAL_API_PUBLIC_BASE_CANCELLATION_OBSERVER_H_
On 2013/09/05 16:06:35, akalin wrote:
> worth pointing out that cancelable etc. seems to be more common in chromium
than
> cancellable, etc. (I don't care either way.)

Although it looks very wrong to me, I should be using American spelling.  Fixed.

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
File sync/internal_api/public/base/cancellation_signal.cc (right):

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
sync/internal_api/public/base/cancellation_signal.cc:14: handler_(NULL){ }
On 2013/09/05 16:06:35, akalin wrote:
> space after )

Done.

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
File sync/internal_api/public/base/cancellation_signal.h (right):

https://codereview.chromium.org/23189021/diff/13001/sync/internal_api/public/...
sync/internal_api/public/base/cancellation_signal.h:55: // registered handler,
if one exists.
On 2013/09/05 16:06:35, akalin wrote:
> might want to mention that OnStopRequested() is called under lock.

Done.

[tim (not reviewing), 2013-09-06 21:47:34]

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.cc (right):

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:132: // We should be registered iff
connection_.get() != NULL.
Is this true in tests as well?  The SCM::MakeConnection returns NULL?

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:133: if (connection_) {
I'd rather use connection_.get() so the scoped_ptr is clear or (better, I think)
explicitly do connection_.reset() below, it's just less subtlety in already
tricky code.

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.h (right):

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:188: CancelationSignal* const
cancelation_signal);
I think you can omit the *const here and keep it in the impl only?  It's weird
to see here since it's useless to the caller, right?

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
File sync/internal_api/public/base/cancelation_observer.cc (right):

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
sync/internal_api/public/base/cancelation_observer.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
earth-shattering nit: remove the (c), per
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Copyri...

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
File sync/internal_api/public/base/cancelation_observer.h (right):

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
sync/internal_api/public/base/cancelation_observer.h:17: virtual void
OnStopRequested() = 0;
Add a comment that this is invoked while holding a lock, and that the Observer
should try to return as quickly as possible.

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
File sync/internal_api/public/base/cancelation_signal.cc (right):

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
sync/internal_api/public/base/cancelation_signal.cc:48:
handler_->OnStopRequested();
A basic test that exercises each method in the intended fashion (from a separate
thread when applicable) would be useful to have, especially running under ASAN,
in case we ever touch this code in the future.

[rlarocque, 2013-09-06 22:42:53]

Patch updated.  PTAL.

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.cc (right):

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:132: // We should be registered iff
connection_.get() != NULL.
On 2013/09/06 21:47:34, timsteele wrote:
> Is this true in tests as well?  The SCM::MakeConnection returns NULL?

Good point.  I wasn't thinking about the tests.  I've added a special case to
the constructor to handle it.

Ideally, production code shouldn't have to care about test behavior, but let's
save that issue for another day.

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:133: if (connection_) {
On 2013/09/06 21:47:34, timsteele wrote:
> I'd rather use connection_.get() so the scoped_ptr is clear or (better, I
think)
> explicitly do connection_.reset() below, it's just less subtlety in already
> tricky code.

I've changed this to connection_.get().

What do you mean by explicit connection_.reset()?  I don't see anything wrong
with having the scoped_ptr cleared in the destructor.

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.h (right):

https://codereview.chromium.org/23189021/diff/33001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.h:188: CancelationSignal* const
cancelation_signal);
On 2013/09/06 21:47:34, timsteele wrote:
> I think you can omit the *const here and keep it in the impl only?  It's weird
> to see here since it's useless to the caller, right? 

Yep, this is the result of a regex gone wrong.  I thought I had fixed all
instances, but I guess I missed this one.

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
File sync/internal_api/public/base/cancelation_observer.cc (right):

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
sync/internal_api/public/base/cancelation_observer.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2013/09/06 21:47:34, timsteele wrote:
> earth-shattering nit: remove the (c), per
>
https://groups.google.com/a/chromium.org/forum/#%21searchin/chromium-dev/Copy...

Fixed here and in three other files.

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
File sync/internal_api/public/base/cancelation_observer.h (right):

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
sync/internal_api/public/base/cancelation_observer.h:17: virtual void
OnStopRequested() = 0;
On 2013/09/06 21:47:34, timsteele wrote:
> Add a comment that this is invoked while holding a lock, and that the Observer
> should try to return as quickly as possible.

Done.

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
File sync/internal_api/public/base/cancelation_signal.cc (right):

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
sync/internal_api/public/base/cancelation_signal.cc:48:
handler_->OnStopRequested();
On 2013/09/06 21:47:34, timsteele wrote:
> A basic test that exercises each method in the intended fashion (from a
separate
> thread when applicable) would be useful to have, especially running under
ASAN,
> in case we ever touch this code in the future.

There are several shutdown scenarios tested in
syncapi_server_connection_manager_unittest.cc.

I know they don't test this as a general-purpose class, but, until we start
using it as a general-purpose class, I don't think such tests are necessary. 
It's more important to test this code as it is actually used, which is what the
existing unit tests do.

[tim (not reviewing), 2013-09-06 23:17:41]

mostly looks good although I still think the test comment stands.

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
File sync/internal_api/public/base/cancelation_signal.cc (right):

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
sync/internal_api/public/base/cancelation_signal.cc:48:
handler_->OnStopRequested();
On 2013/09/06 22:42:53, rlarocque wrote:
> On 2013/09/06 21:47:34, timsteele wrote:
> > A basic test that exercises each method in the intended fashion (from a
> separate
> > thread when applicable) would be useful to have, especially running under
> ASAN,
> > in case we ever touch this code in the future.
> 
> There are several shutdown scenarios tested in
> syncapi_server_connection_manager_unittest.cc.
> 
> I know they don't test this as a general-purpose class, but, until we start
> using it as a general-purpose class, I don't think such tests are necessary. 
> It's more important to test this code as it is actually used, which is what
the
> existing unit tests do.

I don't think it's true that unit tests are unnecessary until we use this class
elsewhere.  The onus shouldn't be on a new customer of this class to create
tests from scratch.  In fact lack of tests should detract any future use of this
class, and certainly of making it general purpose.  I'm not saying there's
always a case for testing helper objects either, but I think you've got a good
class here with well scoped responsibilities and a non-trivial aspect to it
given threading interplay.

[rlarocque, 2013-09-09 18:06:58]

Added tests.  PTAL.

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
File sync/internal_api/public/base/cancelation_signal.cc (right):

https://codereview.chromium.org/23189021/diff/33001/sync/internal_api/public/...
sync/internal_api/public/base/cancelation_signal.cc:48:
handler_->OnStopRequested();
On 2013/09/06 23:17:41, timsteele wrote:
> On 2013/09/06 22:42:53, rlarocque wrote:
> > On 2013/09/06 21:47:34, timsteele wrote:
> > > A basic test that exercises each method in the intended fashion (from a
> > separate
> > > thread when applicable) would be useful to have, especially running under
> > ASAN,
> > > in case we ever touch this code in the future.
> > 
> > There are several shutdown scenarios tested in
> > syncapi_server_connection_manager_unittest.cc.
> > 
> > I know they don't test this as a general-purpose class, but, until we start
> > using it as a general-purpose class, I don't think such tests are necessary.

> > It's more important to test this code as it is actually used, which is what
> the
> > existing unit tests do.
> 
> I don't think it's true that unit tests are unnecessary until we use this
class
> elsewhere.  The onus shouldn't be on a new customer of this class to create
> tests from scratch.  In fact lack of tests should detract any future use of
this
> class, and certainly of making it general purpose.  I'm not saying there's
> always a case for testing helper objects either, but I think you've got a good
> class here with well scoped responsibilities and a non-trivial aspect to it
> given threading interplay.

I may have miscommunicated my argument here.

The CancelationSignal doesn't provide a lot of interesting guarantees, and many
of its most important guarantees are inherently racy.  It will be difficult to
write useful, non-trivial tests for this class.

The tests in syncapi_server_connection_manager_unittest.cc are more useful
because they can detect not only bugs in the CancelationSignal, but also bugs
that may be related to the misuse of the CancelationSignal.  I think the latter
are much more likely, because it's difficult to use this class properly.

That said, I suppose more tests can't hurt.  I added some tests in
cancelation_signal_unittest.cc.  I'm not sure that these tests will be
particularly effective.  Please let me know if you can think of better ways to
test this class.

[tim (not reviewing), 2013-09-09 18:46:34]

https://codereview.chromium.org/23189021/diff/68001/sync/internal_api/public/...
File sync/internal_api/public/base/cancelation_signal_unittest.cc (right):

https://codereview.chromium.org/23189021/diff/68001/sync/internal_api/public/...
sync/internal_api/public/base/cancelation_signal_unittest.cc:105: BlockingTask
blocking_task_;
I don't see anything ever call event_.Reset yet it seems this BlockingTask's
lifetime can span multiple tests, and it is initialized to manual_reset=true.  I
think that would break any future call to VerifyTaskDone.

[tim (not reviewing), 2013-09-09 19:13:59]

On 2013/09/09 18:46:34, timsteele wrote:
>
https://codereview.chromium.org/23189021/diff/68001/sync/internal_api/public/...
> File sync/internal_api/public/base/cancelation_signal_unittest.cc (right):
> 
>
https://codereview.chromium.org/23189021/diff/68001/sync/internal_api/public/...
> sync/internal_api/public/base/cancelation_signal_unittest.cc:105: BlockingTask
> blocking_task_;
> I don't see anything ever call event_.Reset yet it seems this BlockingTask's
> lifetime can span multiple tests, and it is initialized to manual_reset=true. 
I
> think that would break any future call to VerifyTaskDone.

Hm, I was confused.  I originally wanted to ask why you chose to make it
manual_reset, given that you don't reset, and it's not clear that VerifyTaskDone
doesn't clean up after itself.  But the comment about across *tests* is
wrong/misleading.

[tim (not reviewing), 2013-09-09 19:37:39]

LGTM

[commit-bot: I haz the power, 2013-09-09 19:54:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/23189021/68001

[commit-bot: I haz the power, 2013-09-09 20:20:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-09 21:10:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/23189021/88001

[commit-bot: I haz the power, 2013-09-10 00:39:55]

Change committed as 222154