Repository: google/mobly

Issue:
Avoid exception in registration of service when the alias and class are the same and show warning only
From [`service_manager.py`](https://github.com/google/mobly/blob/master/mobly/controllers/android_device_lib/service_manager.py), it provides method `register` to register service:
```python
  def register(self, alias, service_class, configs=None, start_service=True):
    """Registers a service.

    This will create a service instance, starts the service, and adds the
    instance to the mananger.

    Args:
      alias: string, the alias for this instance.
      service_class: class, the service class to instantiate.
      configs: (optional) config object to pass to the service class's
        constructor.
      start_service: bool, whether to start the service instance or not.
        Default is True.
    """
    if not inspect.isclass(service_class):
      raise Error(self._device, '"%s" is not a class!' % service_class)
    if not issubclass(service_class, base_service.BaseService):
      raise Error(
          self._device,
          'Class %s is not a subclass of BaseService!' % service_class,
      )
    if alias in self._service_objects:
      raise Error(
          self._device,
          'A service is already registered with alias "%s".' % alias,
      )
    ...
```
For some case, different layers of testing will try to register the same service (e.g. `uiautomator`) and cause `Error` which is annoying and too strict somehow. So I propose to enhance the checking condition to just show warning when the input `alias` and `service_class` are actually the same of the registered service object. e.g.:
```python
  def register(self, alias, service_class, configs=None, start_service=True):
    """Registers a service.

    This will create a service instance, starts the service, and adds the
    instance to the mananger.

    Args:
      alias: string, the alias for this instance.
      service_class: class, the service class to instantiate.
      configs: (optional) config object to pass to the service class's
        constructor.
      start_service: bool, whether to start the service instance or not.
        Default is True.
    """
    if not inspect.isclass(service_class):
      raise Error(self._device, '"%s" is not a class!' % service_class)
    if not issubclass(service_class, base_service.BaseService):
      raise Error(
          self._device,
          'Class %s is not a subclass of BaseService!' % service_class,
      )
    # New code here
    if alias in self._service_objects and self._service_objects[alias].__class__ == service_class:
      logging.warning('Service with alias=%s has been registered as %s!', alias, service_class)
      return
    if alias in self._service_objects:
      raise Error(
          self._device,
          'A service is already registered with alias "%s".' % alias,
      )
```

So we don't need to add code snippet all the places to ensure we don't register same service more than one time. e.g.:
```python
    if 'uiautomator' not in self._ad.services.list_live_services():
      self._ad.services.register(
          uiautomator.ANDROID_SERVICE_NAME,
          uiautomator.UiAutomatorService)
```

Discussion:
@mhaoli: Instead of letting different layers register the same service, why not putting all the service registeration logic into one place?@johnklee: @mhaoli ,

Good question! Actually, for our code base, we did put all the registration of services in one place. However, we couldn't avoid the other utilities (third-party) to register what ever services they want at what ever time they prefer. 

Ps. Currently, our temporary approach is to unregister the uiautomator service right after instantiate the target object to avoid confliction at later time from our code base to register the uiautomator (Or we could also improve the registration process to check for duplicate registration as well.)@mhaoli: If so, I'd suggest asking the utility owner to check for duplicate registration.

My main concern for your proposal is that it's in the base_service class, so this modification means duplicate registration is allowed and ignored **for all services**. This is true for uiatuomator, but not true for all services, e.g. service for screen recording, service for starting a user session with specific arguments. For these services, duplicate registration should trigger an error.@johnklee: Hi @mhaoli ,

Your concern and comment is fair and well understood. What if we could provide one more argument to let user decides the behavior they want such as a `allow_duplicate`? So we could decide which service we want to avoid exception and allow duplicate registration. e.g.:
```python
def register(
    self, alias, service_class, configs=None, start_service=True, allow_duplicate = False):
    """Registers a service.

    This will create a service instance, starts the service, and adds the
    instance to the mananger.

    Args:
      alias: string, the alias for this instance.
      service_class: class, the service class to instantiate.
      configs: (optional) config object to pass to the service class's
        constructor.
      start_service: bool, whether to start the service instance or not.
        Default is True.
      allow_duplicate: bool, True to ignore the registration of duplicate service.
    """
    if not inspect.isclass(service_class):
      raise Error(self._device, '"%s" is not a class!' % service_class)
    if not issubclass(service_class, base_service.BaseService):
      raise Error(
          self._device,
          'Class %s is not a subclass of BaseService!' % service_class,
      )
    # New code here
    if all([
        alias in self._service_objects,
        self._service_objects[alias].__class__ == service_class,
        allow_duplicate]):
      logging.warning('Service with alias=%s has been registered as %s!', alias, service_class)
      return
    ...
```@mhaoli: I'm ok with the new proposal if you are willing to create a PR for it.

@xpconanfan any objection to this public API change: Add an arg to ignore the registration error if the service class is already registered with the given alias.@xpconanfan: Doesn't look like something worth changing public apis for?
Just catch the exception in your own code and handle it?

On Wed, Apr 17, 2024, 7:59 PM Minghao Li ***@***.***> wrote:

> I'm ok with the new proposal if you are willing to create a PR for it.
>
> @xpconanfan <https://github.com/xpconanfan> any objection to this public
> API change: Add an arg to ignore the registration error if the service
> class is already registered with the given alias.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/google/mobly/issues/919#issuecomment-2062904437>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AARDNZMCZK43VLKXHXFOMWLY54ZIJAVCNFSM6AAAAABGG4RNXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRSHEYDINBTG4>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
@johnklee: Hi @xpconanfan ,

For `Just catch the exception in your own code and handle it?`
> That will work. But the try/catch or the code to examine if the `uiautomator` is already registered will be duplicate in many places. Because there might be more than one third party tools that will use `uiautomator` or even more other services that our testing will register too and cause confliction. So with this feature, we could use `allow_duplicate = True` to ignore those confliction in the root place.

Then for `Doesn't look like something worth changing public apis for?`
> I think with feature, we could save a lot of duplicate code such as below code snippet (Or to add a complex try/catch to ensure the Error is caused by duplicate registration). 
```python
    if 'uiautomator' in self._ad.services.list_live_services():
      # Skip registration of 'uiautomator'
```


If it looks good to both of you, I will prepare a PR. Thanks!@xpconanfan: This is more reason to not allow dup names.

We are not changing public API for this.

It seems like you can easily just check and unregister, then register again.

On Thu, Apr 18, 2024, 1:24 AM John ***@***.***> wrote:

> Hi @xpconanfan <https://github.com/xpconanfan> ,
>
> For Just catch the exception in your own code and handle it?
>
> That will work. But the try/catch or the code to examine if the
> uiautomator is already registered will be duplicate in many places.
> Because there might be more than one third party tools that will use
> uiautomator or even more other services that our testing will register
> too and cause confliction. So with this feature, we could use allow_duplicate
> = True to ignore those confliction in the root place.
>
> Then for Doesn't look like something worth changing public apis for?
>
> I think with feature, we could save a lot of duplicate code such as below
> code snippet (Or to add a complex try/catch to ensure the Error is caused
> by duplicate registration).
>
>     if 'uiautomator' in self._ad.services.list_live_services():
>       # Skip registration of 'uiautomator'
>
> If it looks good to both of you, I will prepare a PR. Thanks!
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/google/mobly/issues/919#issuecomment-2063308037>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AARDNZKKNPAO7FZU7EB6BW3Y557KLAVCNFSM6AAAAABGG4RNXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRTGMYDQMBTG4>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
@johnklee: Hi @xpconanfan ,

What with accessing private field (`self._ad.services`) isn't a good practice, what with the default value of new argument `allow_duplicate` is given as False which aligns with original behavior, I believe the original users won't be impacted. But the benefit is that we could use a more native way to get rid of this dup-registration confliction. Does it make sense to you? Thanks.@xpconanfan: Not sure what you mean
`services` is not private.
`_ad` is "private only bc you made it so, which doesn't make sense in the
first place

On Thu, Apr 18, 2024 at 5:49 PM John ***@***.***> wrote:

> Hi @xpconanfan <https://github.com/xpconanfan> ,
>
> What with accessing private field (self._ad.services) isn't a good
> practice, what with the default value of new argument allow_duplicate is
> given as False which aligns with original behavior, I believe the original
> users won't be impacted. But the benefit is that we could use a more native
> way to get rid of this dup-registration confliction. Does it make sense to
> you? Thanks.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/google/mobly/issues/919#issuecomment-2065553533>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AARDNZK3NFJLDGOTB3G6NUTY6BS2LAVCNFSM6AAAAABGG4RNXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRVGU2TGNJTGM>
> .
> You are receiving this because you were mentioned.Message ID:
> <google/mobly/issues/919/2065553533 ***@***.***>
>
@johnklee: Hi @xpconanfan ,

You are right about `"services" is not private. "_ad" is "private only bc you made it so, which doesn't make sense in the` . I made wrong assumption here. It should could be improved by our inner codebase. 

If support a new argument doesn't sound like a good idea to you, let's return to the original issue, is it possible to let **`ServiceManager`** support query of the registered alias name of a certain service class. Because below code snippet
```python
if 'uiautomator' in self._ad.services.list_live_services():
      # Skip registration of 'uiautomator'
```
will still have issue while the UI automator isn't registered with the name as `uiautomator`. 

So one idea is to have new method such as:
```python
>>> dut = bttc.get('07311JECB08252', init_snippet_uiautomator=True)
>>> from snippet_uiautomator import uiautomator
>>> dut.services.get_service_name_by_class(uiautomator.UiAutomatorService)
'uiautomator'
```

PoC code of method `get_service_name_by_class`:
```python
  def get_service_name_by_class(self, service_class) -> str | None:
    """Gets service name by service class."""
    for alias, service_object in self._service_objects.items():
      if service_object.__class__ == service_class:
        return alias

    return None
```

So we could use this method to confirm the registration of `uiautomator.UiAutomatorService` and confirm the registered name is as what we expected.@xpconanfan: @johnklee The service manager already has `has_service_by_name` so you can easily do `self.ad.services.has_service_by_name('uiautomator')` right?@johnklee: @xpconanfan 

Many thanks for the kind help. Yes, indeed this API could provide us better flexibility to resolve the problem we faced. Let me close the issue as well.

Solution patch:
diff --git a/mobly/controllers/android_device_lib/service_manager.py b/mobly/controllers/android_device_lib/service_manager.py
index 17d5a1f..08fbaa1 100644
--- a/mobly/controllers/android_device_lib/service_manager.py
+++ b/mobly/controllers/android_device_lib/service_manager.py
@@ -122,6 +122,25 @@ class ServiceManager:
       ):
         func(self._service_objects[alias])
 
+  def get_service_alias_by_class(self, service_class):
+    """Gets the aslias name of a registered service.
+
+    The same service class can be registered multiple times with different
+    aliases. When not well managed, duplication and race conditions can arise.
+    One can use this API to de-duplicate as needed.
+
+    Args:
+      service_class: class, the class of a service type.
+
+    Returns:
+      list of strings, the aliases the service is registered with.
+    """
+    aliases = []
+    for alias, service_object in self._service_objects.items():
+      if isinstance(service_object, service_class):
+        aliases.append(alias)
+    return aliases
+
   def list_live_services(self):
     """Lists the aliases of all the services that are alive.
 


Test patch:
diff --git a/tests/mobly/controllers/android_device_lib/service_manager_test.py b/tests/mobly/controllers/android_device_lib/service_manager_test.py
index 471c83f..2d608d3 100755
--- a/tests/mobly/controllers/android_device_lib/service_manager_test.py
+++ b/tests/mobly/controllers/android_device_lib/service_manager_test.py
@@ -469,6 +469,14 @@ class ServiceManagerTest(unittest.TestCase):
     with self.assertRaisesRegex(service_manager.Error, msg):
       manager.resume_services(['mock_service'])
 
+  def test_get_alias_by_class(self):
+    manager = service_manager.ServiceManager(mock.MagicMock())
+    manager.register('mock_service1', MockService, start_service=False)
+    manager.register('mock_service2', MockService, start_service=False)
+    manager.start_services(['mock_service2'])
+    aliases = manager.get_service_alias_by_class(MockService)
+    self.assertEqual(aliases, ['mock_service1', 'mock_service2'])
+
 
 if __name__ == '__main__':
   unittest.main()