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

Issue 672863 link

Starred by 1 user
Status: Fixed
Owner:
blundell@chromium.org
Closed: Dec 2016
Cc:
█ roc...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Eliminate ServiceManager using parent-child relationship for services

Project Member Reported by blundell@chromium.org, Dec 9 2016 
Currently, ServiceManager has the following ownership model for per-user services:

A per-user service (Service B) is owned by the first service to connect to it (Service A). If Service A dies, Service B is killed as well.

This parent-child relationship makes sense in some contexts but does not in others, e.g., suppose Service B is a service connected to by all renderers: it will die when the first renderer to connect to it is killed, even if there are other renderers that still need it to be alive. For more details on this use case, see this chromium-mojo@ thread:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/41Bhbnk6CY0

We will eliminate this default ownership model and instead have the following:

- By default, services will be completely responsible for their own lifetime.
- A service can specify that it is a "session manager" in its manifest. Such a service will own all the services that it directly or transitively connects to.
- We will use this "session manager" attribute to keep the current behavior in cases where it is desired.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 15 2016 

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

commit 7bc63b9913f48759d9ac0004a1198b307209985a
Author: blundell <blundell@chromium.org>
Date: Thu Dec 15 08:04:42 2016

[ServiceManager] Eliminate parent-child relationship between services

This change eliminates the property that non-singleton services are
owned by the first service that connects to them. Instead, singleton
and non-singleton services are treated identically by the Service
Manager: they go away either (1) when the service requests so or
(2) when the Service Manager itself goes away.

The motivation for this change is that (a) there are situations where
this "tree ownership" model doesn't make sense (see linked bug), and
(b) there is nothing currently in the codebase that is utilizing the
tree ownership model. If desired, the tree ownership model could be
resurrected on a per-service basis by adding a "session manager" attribute
to a service's manifest, which would denote that that service should own
all of the services that it directly or transitively connects to.

This CL also changes Service Manager's destructor to perform
two-phase shutdown on its owned Instances. This change eliminates
possible deadlock in the following situation:
- An Instance A blocks in its destructor for its Runner to complete
- That Runner blocks completion on the corresponding Service A shutting down
- Service A blocks its shutdown on some distinct Service B shutting down
- Service B is waiting to receive a connection error in order to initiate
  shutdown
- ServiceManager never deletes Instance B since it is blocked waiting for
  the destructor of Instance A to return

An example of such a situation is found in connect_test_package.cc:
ConnectTestService clears its ProvidedService instances in its OnStop() method,
while ProvidedService does not exit its destructor until it receives a
connection error. Before this CL, deadlock was not tickled because the
ProvidedServices were always killed when the first service that
connected to them was killed, which happened to be before ConnectTestService
was killed.

BUG= 672863 

Review-Url: https://codereview.chromium.org/2572803002
Cr-Commit-Position: refs/heads/master@{#438780}

[modify] https://crrev.com/7bc63b9913f48759d9ac0004a1198b307209985a/services/service_manager/service_manager.cc
[modify] https://crrev.com/7bc63b9913f48759d9ac0004a1198b307209985a/services/service_manager/service_manager.h
[modify] https://crrev.com/7bc63b9913f48759d9ac0004a1198b307209985a/services/service_manager/tests/lifecycle/lifecycle_unittest.cc

Comment 2 by blundell@chromium.org, Dec 29 2016

Status: Fixed (was: Started)
►

Sign in to add a comment