Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2572803002:
[ServiceManager] Eliminate parent-child relationship between services (Closed)
|
Created:
4 years ago by blundell Modified:
4 years ago Reviewers:
Ken Rockot(use gerrit already) CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[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
Committed: https://crrev.com/7bc63b9913f48759d9ac0004a1198b307209985a
Cr-Commit-Position: refs/heads/master@{#438780}
Patch Set 1 #Patch Set 2 : More general fix for ConnectTest shutdown deadlock #
Messages
Total messages: 24 (19 generated)
Description was changed from ========== Eliminate parent-child relationship between services ========== to ========== [ServiceManager] Eliminate parent-child relationship between services ==========
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ServiceManager] Eliminate parent-child relationship between services ========== to ========== [ServiceManager] Eliminate parent-child relationship between services BUG=672863 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ServiceManager] Eliminate parent-child relationship between services BUG=672863 ========== to ========== [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, (2) when all connections are lost, or (3) 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. As part of this change, Service Manager's destructor is changed to perform two-phase shutdown on its owned Instances. This change is made to eliminate possible deadlock due to an Instance A waiting in its destructor for its Runner to complete, while that Runner is waiting for the corresponding Service A to shut down, while Service A is waiting for some distinct Service B to shut down, while Service B is waiting to receive a connection error in order to initiate shutdown. BUG=672863 ==========
Description was changed from ========== [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, (2) when all connections are lost, or (3) 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. As part of this change, Service Manager's destructor is changed to perform two-phase shutdown on its owned Instances. This change is made to eliminate possible deadlock due to an Instance A waiting in its destructor for its Runner to complete, while that Runner is waiting for the corresponding Service A to shut down, while Service A is waiting for some distinct Service B to shut down, while Service B is waiting to receive a connection error in order to initiate shutdown. BUG=672863 ========== to ========== [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. As part of this change, Service Manager's destructor is changed to perform two-phase shutdown on its owned Instances. This change is made to eliminate possible deadlock due to an Instance A waiting in its destructor for its Runner to complete, while that Runner is waiting for the corresponding Service A to shut down, while Service A is waiting for some distinct Service B to shut down, while Service B is waiting to receive a connection error in order to initiate shutdown. BUG=672863 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [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. As part of this change, Service Manager's destructor is changed to perform two-phase shutdown on its owned Instances. This change is made to eliminate possible deadlock due to an Instance A waiting in its destructor for its Runner to complete, while that Runner is waiting for the corresponding Service A to shut down, while Service A is waiting for some distinct Service B to shut down, while Service B is waiting to receive a connection error in order to initiate shutdown. BUG=672863 ========== to ========== [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 due to an Instance A waiting in its destructor for its Runner to complete, while that Runner is waiting for the corresponding Service A to shut down, while Service A is waiting for some distinct Service B to shut down, while Service B is waiting to receive a connection error in order to initiate shutdown. 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 avoided 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 ==========
Description was changed from ========== [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 due to an Instance A waiting in its destructor for its Runner to complete, while that Runner is waiting for the corresponding Service A to shut down, while Service A is waiting for some distinct Service B to shut down, while Service B is waiting to receive a connection error in order to initiate shutdown. 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 avoided 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 ========== to ========== [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 ==========
blundell@chromium.org changed reviewers: + rockot@chromium.org
The actual change took very little time, but debugging the ConnectTest hang that resulted took a little while.
lgtm
The CQ bit was checked by blundell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481785613198830,
"parent_rev": "105524a0b2a5c6d2563928dfab228f3fee2b6e5d", "commit_rev":
"1503d9a8bbae64f28a6be3ccc9f87ed35e57b494"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 Committed: https://crrev.com/7bc63b9913f48759d9ac0004a1198b307209985a Cr-Commit-Position: refs/heads/master@{#438780} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7bc63b9913f48759d9ac0004a1198b307209985a Cr-Commit-Position: refs/heads/master@{#438780} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
