sync: Gracefully handle very early shutdown

sync/internal_api/public/base/cancelation_signal.cc

Index: sync/internal_api/public/base/cancelation_signal.cc
diff --git a/sync/internal_api/public/base/cancelation_signal.cc b/sync/internal_api/public/base/cancelation_signal.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a95578f5fa9baf87c2bdbba91f51ca18b2b5974e
--- /dev/null
+++ b/sync/internal_api/public/base/cancelation_signal.cc
@@ -0,0 +1,52 @@
+// Copyright (c) 2013 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/internal_api/public/base/cancelation_signal.h"
+
+#include "base/logging.h"
+#include "sync/internal_api/public/base/cancelation_observer.h"
+
+namespace syncer {
+
+CancelationSignal::CancelationSignal()
+  : stop_requested_(false),
+    handler_(NULL) { }
+
+CancelationSignal::~CancelationSignal() {
+  DCHECK(!handler_);
+}
+
+bool CancelationSignal::TryRegisterHandler(CancelationObserver* handler) {
+  base::AutoLock lock(stop_requested_lock_);
+  DCHECK(!handler_);
+
+  if (stop_requested_)
+    return false;
+
+  handler_ = handler;
+  return true;
+}
+
+void CancelationSignal::UnregisterHandler(CancelationObserver* handler) {
+  base::AutoLock lock(stop_requested_lock_);
+  DCHECK_EQ(handler_, handler);
+  handler_ = NULL;
+}
+
+bool CancelationSignal::IsStopRequested() {
+  base::AutoLock lock(stop_requested_lock_);
+  return stop_requested_;
+}
+
+void CancelationSignal::RequestStop() {
+  base::AutoLock lock(stop_requested_lock_);
+  DCHECK(!stop_requested_);
+
+  stop_requested_ = true;
+  if (handler_) {
+    handler_->OnStopRequested();

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

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]

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]

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]

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.

+  }
+}
+
+}  // namespace syncer