Eliminate Connector::Connect(), Connection, etc.

Eliminate Connector::Connect(), Connection, etc.

. Introduce a new Connector method StartService(Identity id); which allows a service to be started by the service manager but no connection to be made to it.
. Migrates service manager tests away from Connection, OnConnect etc.
. Eliminate Connector::Connect(), Service::OnConnect() & logic in ServiceManager supporting them.
. Eliminate the Connection object & remove #includes
. Change how Connector response callbacks are run to guarantee they're reliably called. (Called upon destruction of ConnectParams).

R=rockot@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2804373002
Cr-Commit-Position: refs/heads/master@{#464167}
Committed: https://chromium.googlesource.com/chromium/src/+/aad37ecd104724b8fae303f241c9ce8af45a1fc1

[Ben Goodger (Google), 2017-04-10 23:59:25]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 00:00:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 00:10:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 00:10:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ben Goodger (Google), 2017-04-11 02:44:10]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 02:44:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 02:54:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 02:54:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Ben Goodger (Google), 2017-04-11 03:05:21]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 03:05:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 03:17:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 03:17:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2017-04-11 03:19:19]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 03:19:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 03:43:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 03:43:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2017-04-11 03:55:27]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 03:55:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 04:04:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 04:04:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[Ben Goodger (Google), 2017-04-11 04:09:26]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 04:09:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 04:29:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 04:29:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2017-04-11 05:01:07]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 05:01:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2017-04-11 05:13:11]

Description was changed from

==========
Migrate remaining users of Connector::Connect() to Connector::StartService() &
Connector::BindInterface()

BUG=
==========

to

==========
Eliminate Connector::Connect(), Connection, etc.

. Introduce a new Connector method StartService(Identity id); which allows a
service to be started by the service manager but no connection to be made to it.
. Migrates service manager tests away from Connection, OnConnect etc.
. Eliminate Connector::Connect(), Service::OnConnect() & logic in ServiceManager
supporting them.
. Eliminate the Connection object & remove #includes
. Change how Connector response callbacks are run to guarantee they're reliably
called. (Called upon destruction of ConnectParams).

R=rockot@chromium.org,tsepez@chromium.org
==========

[Ben Goodger (Google), 2017-04-11 05:13:12]

ben@chromium.org changed reviewers:
+ rockot@chromium.org, tsepez@chromium.org

[Ben Goodger (Google), 2017-04-11 05:31:02]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 05:31:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 05:58:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 05:58:12]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Ben Goodger (Google), 2017-04-11 20:20:54]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 20:21:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2017-04-11 21:18:12]

OK. Sorry this looks large, most of the effort is in //services/service_manager.

Tom: *.mojom. This is basically the last step in a sequence I told you about a
while ago where we move from having services enforce capability policy to one
where the service manager does it. I'm removing the Connect() method which
allows for capabilities to be exchanged directly without involving the service
manager. Also changed function order in the interfaces & updated the
documentation.

Ken: *

[commit-bot: I haz the power, 2017-04-11 22:21:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 22:21:02]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-04-11 23:29:44]

LGTM.

BTW I'm not entirely sure that StartService is something we want to encourage in
the long term.

It's easy to misuse and essentially only makes sense for long-running services
that don't limit their lifetime by client usage. e.g. if you were to
StartService("data_decoder") in production right now, you'd launch a new service
process that would sit idle until someone came along and actually used the
data_decoder for something, which may be never. I don't think it's a problem
yet, but something to think about.

https://codereview.chromium.org/2804373002/diff/240001/services/service_manag...
File services/service_manager/connect_params.cc (right):

https://codereview.chromium.org/2804373002/diff/240001/services/service_manag...
services/service_manager/connect_params.cc:11: if
(!start_service_callback_.is_null())
this is pretty goofy but i guess it's alright for now. better than accidentally
not responding.

[Ben Goodger (Google), 2017-04-11 23:30:52]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2017-04-11 23:30:56]

The CQ bit was unchecked by ben@chromium.org

[Ben Goodger (Google), 2017-04-11 23:33:23]

On 2017/04/11 23:29:44, Ken Rockot wrote:
> LGTM.
> 
> BTW I'm not entirely sure that StartService is something we want to encourage
in
> the long term.
> 
> It's easy to misuse and essentially only makes sense for long-running services
> that don't limit their lifetime by client usage. e.g. if you were to
> StartService("data_decoder") in production right now, you'd launch a new
service
> process that would sit idle until someone came along and actually used the
> data_decoder for something, which may be never. I don't think it's a problem
> yet, but something to think about.

I think we tend to use it in "bootstrap" cases. I wonder if it's something that
requires a special capability like the one with a ClientProcessConnection data.

>
https://codereview.chromium.org/2804373002/diff/240001/services/service_manag...
> File services/service_manager/connect_params.cc (right):
> 
>
https://codereview.chromium.org/2804373002/diff/240001/services/service_manag...
> services/service_manager/connect_params.cc:11: if
> (!start_service_callback_.is_null())
> this is pretty goofy but i guess it's alright for now. better than
accidentally
> not responding.

I decided this was elegant, though it may be that some of the class/method names
should change. I decided that every in-flight Connector "operation" owns one of
these Params objects & when the Params object is destructed that the operation
is complete & the response callback should be run. Unfortunately there are
enough twists & turns in ServiceManager that I think this is necessary for
simplicity. Perhaps this class could be called something else.

[Tom Sepez, 2017-04-12 16:18:48]

lgtm

https://codereview.chromium.org/2804373002/diff/240001/services/service_manag...
File services/service_manager/public/interfaces/connector.mojom (right):

https://codereview.chromium.org/2804373002/diff/240001/services/service_manag...
services/service_manager/public/interfaces/connector.mojom:105: //    user id to
BindInterface(), which will be resolved by the service manager
nit:80 cols

[Ben Goodger (Google), 2017-04-12 19:17:35]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2017-04-12 19:17:35]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2804373002/#ps260001
(title: ".")

[commit-bot: I haz the power, 2017-04-12 19:18:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 19:34:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 19:34:41]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2017-04-12 19:49:06]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2017-04-12 19:49:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2804373002/#ps280001
(title: ".")

[commit-bot: I haz the power, 2017-04-12 19:50:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 22:08:25]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1492026546762700,
"parent_rev": "08549ea5b3c3af57a2a21c2bcbf2be3ce7160781", "commit_rev":
"aad37ecd104724b8fae303f241c9ce8af45a1fc1"}

[commit-bot: I haz the power, 2017-04-12 22:09:22]

Description was changed from

==========
Eliminate Connector::Connect(), Connection, etc.

. Introduce a new Connector method StartService(Identity id); which allows a
service to be started by the service manager but no connection to be made to it.
. Migrates service manager tests away from Connection, OnConnect etc.
. Eliminate Connector::Connect(), Service::OnConnect() & logic in ServiceManager
supporting them.
. Eliminate the Connection object & remove #includes
. Change how Connector response callbacks are run to guarantee they're reliably
called. (Called upon destruction of ConnectParams).

R=rockot@chromium.org,tsepez@chromium.org
==========

to

==========
Eliminate Connector::Connect(), Connection, etc.

. Introduce a new Connector method StartService(Identity id); which allows a
service to be started by the service manager but no connection to be made to it.
. Migrates service manager tests away from Connection, OnConnect etc.
. Eliminate Connector::Connect(), Service::OnConnect() & logic in ServiceManager
supporting them.
. Eliminate the Connection object & remove #includes
. Change how Connector response callbacks are run to guarantee they're reliably
called. (Called upon destruction of ConnectParams).

R=rockot@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2804373002
Cr-Commit-Position: refs/heads/master@{#464167}
Committed:
https://chromium.googlesource.com/chromium/src/+/aad37ecd104724b8fae303f241c9...
==========

[commit-bot: I haz the power, 2017-04-12 22:09:24]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/aad37ecd104724b8fae303f241c9...

[seantopping, 2017-04-18 22:48:49]

seantopping@chromium.org changed reviewers:
+ seantopping@chromium.org

[seantopping, 2017-04-18 22:48:50]

Hey Ben,

Chromecast has a use case for spinning up and watching services that don't
expose any interfaces; We were previously using a service_manager::Connection to
start an out-of-process "service" and set a connection error handler to restart
the service in case it dies unexpectedly.

Under this new API, what's the preferred method for maintaining this logic?

[Ben Goodger (Google), 2017-04-19 01:46:18]

You invent another interface specific to that purpose.

See for example content::mojom::Child.

-Ben

On Tue, Apr 18, 2017, 3:48 PM <seantopping@chromium.org> wrote:

> Hey Ben,
>
> Chromecast has a use case for spinning up and watching services that don't
> expose any interfaces; We were previously using a
> service_manager::Connection to
> start an out-of-process "service" and set a connection error handler to
> restart
> the service in case it dies unexpectedly.
>
> Under this new API, what's the preferred method for maintaining this logic?
>
> https://codereview.chromium.org/2804373002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.