Services have no way to avoid quit racing with inbound connections |
||
Issue description
Services and ServiceManager must be able to synchronize lifetime expectations in order to avoid races.
Like any typical application, a service may decide to terminate itself under certain conditions (e.g., no longer servicing any outstanding requests.) This is essentially done by breaking its pipe to the ServiceManager and/or signaling to its embedder that the Service impl should be destroyed.
Such actions can race with the ServiceManager sending new inbound connection requests to the service, and these requests contain information from the connecting client that cannot simply be duplicated and retransmitted in the event of failure.
It is therefore necessary for a well-behaved service to notify the ServiceManager of its intent to terminate and wait for confirmation from the ServiceManager before taking any further action. Additionally, this notification/response must happen over a channel which maintains FIFO with the existing Service pipe so that there are no races with OnConnect transmission or dispatch.
There are two reasonable options here. Both require changes to the Service contract, and addition to one of the changes below, OnConnect also must change to expect a (zero-argument) reply.
1. Introduce a ServiceControl interface and give the service a proxy to it in OnStart():
interface ServiceControl {
RequestQuit();
};
This must be associated with the inbound Service pipe. When a service is ready to terminate, it sends RequestQuit(). If the ServiceManager has an outstanding OnConnect (no response received yet), it ignores the request. Otherwise it proceeds with normal instance termination and sends a Service::OnStop().
2. Introduce a new message on Service:
interface Service {
// ...
WaitForQuit() => ();
};
The ServiceManager issues this request immediately after OnStart(). The Service is responsible for retaining the response callback until it's ready to quit, at which point it replies.
If the ServiceManager has an outstanding OnConnect (no response received yet), it simply issues another WaitForQuit() (the Service will receive the pending OnConnect() before receiving this WaitForQuit()). Otherwise it proceeds with normal instance termination and sends Service::OnStop().
IMHO #1 may be easier to read, but #2 avoids associated interfaces which may be a bigger win for such a fundamental interface as Service.
,
Nov 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5b9cb3df6c06015d79aeb688c2b8905016086b9e commit 5b9cb3df6c06015d79aeb688c2b8905016086b9e Author: rockot <rockot@chromium.org> Date: Mon Nov 07 23:27:21 2016 Service Manager: Implement graceful service termination Adds a new ServiceControl client interface associated with a service's Service interface. This allows a service to notify the Service Manager that it's ready to be terminated by calling RequestQuit(). Adds an ack reply to Service.OnConnect. The Service Manager will ignore any RequestQuit() issued while an OnConnect is still pending acknowledgement. Fixes a race in existing service_manager_unittests which was causing some tests to hang fairly regularly, using RequestQuit() to avoid races between service shutdown and incoming connections. Also fixes incorrect base::SimpleThread usage in some tests. BUG=654986, 662177 Review-Url: https://codereview.chromium.org/2480603004 Cr-Commit-Position: refs/heads/master@{#430414} [modify] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/chrome/test/base/mojo_test_connector.cc [modify] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/services/service_manager/public/cpp/lib/service_context.cc [modify] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/services/service_manager/public/cpp/service_context.h [modify] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/services/service_manager/public/interfaces/BUILD.gn [modify] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/services/service_manager/public/interfaces/service.mojom [add] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/services/service_manager/public/interfaces/service_control.mojom [modify] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/services/service_manager/service_manager.cc [modify] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/services/service_manager/tests/connect/connect_test_package.cc [modify] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/services/service_manager/tests/lifecycle/app_client.cc [modify] https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e/services/service_manager/tests/lifecycle/app_client.h
,
Nov 8 2016
,
Nov 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1a665b53fe46aba0c80416b9b354760fc1ccf749 commit 1a665b53fe46aba0c80416b9b354760fc1ccf749 Author: benwells <benwells@chromium.org> Date: Wed Nov 09 05:04:42 2016 Revert of Service Manager: Implement graceful service termination (patchset #1 id:1 of https://codereview.chromium.org/2480603004/ ) Reason for revert: Seems to be the cause of lock order inversions on TSan bots. See https://crbug.com/663557 for details. BUG= 663557 Original issue's description: > Service Manager: Implement graceful service termination > > Adds a new ServiceControl client interface associated with > a service's Service interface. This allows a service to notify > the Service Manager that it's ready to be terminated by calling > RequestQuit(). > > Adds an ack reply to Service.OnConnect. The Service Manager will > ignore any RequestQuit() issued while an OnConnect is still > pending acknowledgement. > > Fixes a race in existing service_manager_unittests which was > causing > some tests to hang fairly regularly, using RequestQuit() > to avoid races between service shutdown and incoming connections. > > Also fixes incorrect base::SimpleThread usage in some tests. > > BUG=654986, 662177 > > Committed: https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e > Cr-Commit-Position: refs/heads/master@{#430414} TBR=ben@chromium.org,tsepez@chromium.org,blundell@chromium.org,rockot@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=654986, 662177 Review-Url: https://codereview.chromium.org/2486073003 Cr-Commit-Position: refs/heads/master@{#430868} [modify] https://crrev.com/1a665b53fe46aba0c80416b9b354760fc1ccf749/chrome/test/base/mojo_test_connector.cc [modify] https://crrev.com/1a665b53fe46aba0c80416b9b354760fc1ccf749/services/service_manager/public/cpp/lib/service_context.cc [modify] https://crrev.com/1a665b53fe46aba0c80416b9b354760fc1ccf749/services/service_manager/public/cpp/service_context.h [modify] https://crrev.com/1a665b53fe46aba0c80416b9b354760fc1ccf749/services/service_manager/public/interfaces/BUILD.gn [modify] https://crrev.com/1a665b53fe46aba0c80416b9b354760fc1ccf749/services/service_manager/public/interfaces/service.mojom [delete] https://crrev.com/3f4b9da10b124cf841a5691abd4ede58f768be42/services/service_manager/public/interfaces/service_control.mojom [modify] https://crrev.com/1a665b53fe46aba0c80416b9b354760fc1ccf749/services/service_manager/service_manager.cc [modify] https://crrev.com/1a665b53fe46aba0c80416b9b354760fc1ccf749/services/service_manager/tests/connect/connect_test_package.cc [modify] https://crrev.com/1a665b53fe46aba0c80416b9b354760fc1ccf749/services/service_manager/tests/lifecycle/app_client.cc [modify] https://crrev.com/1a665b53fe46aba0c80416b9b354760fc1ccf749/services/service_manager/tests/lifecycle/app_client.h
,
Nov 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46 commit 66ee2e33ec55e317dfbf11209b9a4c08ecac4c46 Author: rockot <rockot@chromium.org> Date: Thu Nov 10 21:16:11 2016 Service Manager: Implement graceful service termination Adds a new ServiceControl client interface associated with a service's Service interface. This allows a service to notify the Service Manager that it's ready to be terminated by calling RequestQuit(). Adds an ack reply to Service.OnConnect. The Service Manager will ignore any RequestQuit() issued while an OnConnect is still pending acknowledgement. Fixes a race in existing service_manager_unittests which was causing some tests to hang fairly regularly, using RequestQuit() to avoid races between service shutdown and incoming connections. Also fixes incorrect base::SimpleThread usage in some tests. BUG=654986, 662177 Committed: https://crrev.com/5b9cb3df6c06015d79aeb688c2b8905016086b9e Review-Url: https://codereview.chromium.org/2480603004 Cr-Original-Commit-Position: refs/heads/master@{#430414} Cr-Commit-Position: refs/heads/master@{#431354} [modify] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/chrome/test/base/mojo_test_connector.cc [modify] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/services/service_manager/public/cpp/lib/service_context.cc [modify] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/services/service_manager/public/cpp/service_context.h [modify] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/services/service_manager/public/interfaces/BUILD.gn [modify] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/services/service_manager/public/interfaces/service.mojom [add] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/services/service_manager/public/interfaces/service_control.mojom [modify] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/services/service_manager/service_manager.cc [modify] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/services/service_manager/tests/connect/connect_test_package.cc [modify] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/services/service_manager/tests/lifecycle/app_client.cc [modify] https://crrev.com/66ee2e33ec55e317dfbf11209b9a4c08ecac4c46/services/service_manager/tests/lifecycle/app_client.h |
||
►
Sign in to add a comment |
||
Comment 1 by roc...@chromium.org
, Nov 3 2016