Service Manager: Implement graceful service termination

services/service_manager/service_manager.cc

Index: services/service_manager/service_manager.cc
diff --git a/services/service_manager/service_manager.cc b/services/service_manager/service_manager.cc
index cf7885640b29c737956c34133a516480f590eecb..e0eb0b3a549490209474f7da21904958474b7d62 100644
--- a/services/service_manager/service_manager.cc
+++ b/services/service_manager/service_manager.cc
@@ -19,6 +19,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/trace_event/trace_event.h"
+#include "mojo/public/cpp/bindings/associated_binding.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "services/service_manager/connect_util.h"
@@ -29,6 +30,7 @@
 #include "services/service_manager/public/cpp/service_context.h"
 #include "services/service_manager/public/interfaces/connector.mojom.h"
 #include "services/service_manager/public/interfaces/service.mojom.h"
+#include "services/service_manager/public/interfaces/service_control.mojom.h"
 #include "services/service_manager/public/interfaces/service_manager.mojom.h"
 
 namespace service_manager {
@@ -78,7 +80,8 @@ class ServiceManager::Instance
       public mojom::PIDReceiver,
       public Service,
       public InterfaceFactory<mojom::ServiceManager>,
-      public mojom::ServiceManager {
+      public mojom::ServiceManager,
+      public mojom::ServiceControl {
  public:
   Instance(service_manager::ServiceManager* service_manager,
            const Identity& identity,
@@ -89,6 +92,7 @@ class ServiceManager::Instance
         interface_provider_specs_(interface_provider_specs),
         allow_any_application_(GetConnectionSpec().requires.count("*") == 1),
         pid_receiver_binding_(this),
+        control_binding_(this),
         state_(State::IDLE),
         weak_factory_(this) {
     if (identity_.name() == kServiceManagerName ||
@@ -151,11 +155,20 @@ class ServiceManager::Instance
     Instance* source = service_manager_->GetExistingInstance(params->source());
     if (source)
       specs = source->interface_provider_specs_;
+
+    pending_service_connections_++;
     service_->OnConnect(ServiceInfo(params->source(), specs),
-                        params->TakeRemoteInterfaces());
+                        params->TakeRemoteInterfaces(),
+                        base::Bind(&Instance::OnConnectComplete,
+                                   base::Unretained(this)));
     return true;
   }
 
+  void OnConnectComplete() {
+    DCHECK_GT(pending_service_connections_, 0);
+    pending_service_connections_--;
+  }
+
   void StartWithService(mojom::ServicePtr service) {
     CHECK(!service_);
     state_ = State::STARTING;
@@ -164,7 +177,7 @@ class ServiceManager::Instance
         base::Bind(&Instance::OnServiceLost, base::Unretained(this),
                    service_manager_->GetWeakPtr()));
     service_->OnStart(ServiceInfo(identity_, interface_provider_specs_),
-                      base::Bind(&Instance::OnInitializeResponse,
+                      base::Bind(&Instance::OnStartComplete,
                                  base::Unretained(this)));
   }
 
@@ -411,7 +424,8 @@ class ServiceManager::Instance
     }
   }
 
-  void OnInitializeResponse(mojom::ConnectorRequest connector_request) {
+  void OnStartComplete(mojom::ConnectorRequest connector_request,
+                       mojom::ServiceControlAssociatedRequest control_request) {
     state_ = State::STARTED;
     if (connector_request.is_pending()) {
       connectors_.AddBinding(this, std::move(connector_request));
@@ -419,6 +433,8 @@ class ServiceManager::Instance
           base::Bind(&Instance::OnConnectionLost, base::Unretained(this),
                      service_manager_->GetWeakPtr()));
     }
+    if (control_request.is_pending())
+      control_binding_.Bind(std::move(control_request));
     service_manager_->NotifyServiceStarted(identity_, pid_);
   }
 
@@ -430,6 +446,13 @@ class ServiceManager::Instance
     service_manager_->OnInstanceError(this);
   }
 
+  // mojom::ServiceControl:
+  void RequestQuit() override {
+    // If quit is requested, oblige when there are no pending OnConnects.
+    if (!pending_service_connections_)

[blundell, 2016/11/08 14:12:56]

If there are pending service connections, should this remember the quit request
and oblige when there are no more pending connections? It seems a little strange
that the quit request gets silently dropped on the floor in this case.

+      OnServiceLost(service_manager_->GetWeakPtr());
+  }
+
   service_manager::ServiceManager* const service_manager_;
 
   // An id that identifies this instance. Distinct from pid, as a single process
@@ -445,10 +468,15 @@ class ServiceManager::Instance
   mojo::Binding<mojom::PIDReceiver> pid_receiver_binding_;
   mojo::BindingSet<mojom::Connector> connectors_;
   mojo::BindingSet<mojom::ServiceManager> service_manager_bindings_;
+  mojo::AssociatedBinding<mojom::ServiceControl> control_binding_;
   base::ProcessId pid_ = base::kNullProcessId;
   Instance* parent_ = nullptr;
   InstanceMap children_;
   State state_;
+
+  // The number of outstanding OnConnect requests which are in flight.
+  int pending_service_connections_ = 0;
+
   base::WeakPtrFactory<Instance> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Instance);