Fix Unregister service value error bug

[aaronayres35]

fixes #251

This PR first adds a failing regression test for the error described in the issue.

To fix the issue, I added a listener in the CorePlugin class that listens to the service_registry's unregistered event trait. As mentioned in the issue, when Application.unregister_service(<id>) was called, the id was not also removed from CorePlugin._service_ids, so in the handler we simply remove the unregistered service id from self._service_ids.

[aaronayres35]

My initial attempted solution is flawed as we dont actually want to lister for all unregistered services, but instead only those which were contributed as service offers to the CorePlugin's service_offers extension point.

Also note the comment here:

@on_trait_change("service_offers_items")
    def _service_offers_changed(self, event):
        """ React to new service offers being *added*.

        Note that we don't currently do anything if services are *removed* as
        we have no facility to let users of the service know that the offer
        has been retracted.

        """
        for service in event.added:
            self._register_service_offer(service)

        return

[kitchoi]

The _service_ids is defined in start, but instead of having it reset in stop, we are removing values via a change handler. The asymmetry and complexity in the logic make me wonder if this is the right fix. If this is the right fix, it does not look obviously right. @aaronayres35 Could you explain a bit more why this is done this way please?

[kitchoi]

The asymmetry and complexity in the logic make me wonder if this is the right fix. If this is the right fix, it does not look obviously right.

To elaborate... a more obviously correct fix is to implement stop that reverses what start does. Maybe we can have an _unregister_service_offers on the core plugin and reverse the action of registering service offers... I have not explored if this could work, and maybe that is something you have tried already. But it would be good, for reference, to know if that more obviously correct alternative could fix the issue.

[aaronayres35]

The _service_ids is defined in start, but instead of having it reset in stop, we are removing values via a change handler. The asymmetry and complexity in the logic make me wonder if this is the right fix. If this is the right fix, it does not look obviously right. @aaronayres35 Could you explain a bit more why this is done this way please?

So basically, what happened previously was that _service_ids was defined in start, by registering each service offer that was offered to the service_offers extension point. And there is a comment saying:

# These services are unregistered by the default plugin activation
# strategy (due to the fact that we store the service ids in this
# specific trait!).

So, when application.stop was called it lead to self.plugin_manager.stop() being called which for each plugin called plugin.activator.stop_plugin(plugin). This then calls plugin.stop() and then plugin.unregister_services(). Currently the CorePlugin does not override the base plugin stop method, so this does nothing. plugin.unregister_services() is where the error occurs because if a service_offer is unregistered before this process has started it is not removed from the CorePlugins _service_ids.

For reference, plugin.unregister_services() does the following:

# Unregister the services in the reverse order that we registered
        # them.
        service_ids = self._service_ids[:]
        service_ids.reverse()

        for service_id in service_ids:
            self.application.unregister_service(service_id)

        # Just in case the plugin is started again!
        self._service_ids = []

        return

It would also be a valid approach to define a stop method on the CorePlugin class that tried to remove and service ids for services which were unregistered while the application was running from _service_ids. I thought adding a listener would make more sense as in that case, the _service_ids is actually a kept up-to-date list of the service ids that are currently registered via the service_offers extension point.

Note that if a service offer is not unregistered while the application is running then without any of the changes made in this PR, they will still be unregistered when application.stop is called via the ultimate call to plugin.unregister_services(). The problem only arises when unregistering a service before the application is stopped, as in that case when that happens, service_ids is not updated to match the current state of services that are registered. In that sense I thought a listener was the best way to keep _service_ids always in sync. It may be possible to replace the listener with a stop method on CorePlugin though and try to do the resynchronization then? That seems a bit more confusing to me personally though

[kitchoi]

Thank you @aaronayres35.

Looks like we are having more than one sources of truth here. Both the service registry on the application and the individual plugin are trying to manage the life-time of a service, so they could end up stepping on each other's toes, and causing a service to be unregistered twice (or more times).

Eliminating multiple sources of truth would be a better fix. Otherwise, we are going to have to do more of these sychronization, e.g. not just _service_ids on the CorePlugin requires synchronization, any instances of Plugin that also maintains the service id list will also need to be synchronized.

[kitchoi]

Another alternative approach to consider may be for plugins to anticipate that when it tries to unregister a service, it may have already been unregistered by others, and handle the exception there.

[kitchoi]

Eliminating multiple sources of truth would be a better fix. Otherwise, we are going to have to do more of these sychronization, e.g. not just _service_ids on the CorePlugin requires synchronization, any instances of Plugin that also maintains the service id list will also need to be synchronized.
Another alternative approach to consider may be for plugins to anticipate that when it tries to unregister a service, it may have already been unregistered by others, and handle the exception there.

Ah, this would present a mental shift in understanding... The service ids on the plugins provide no indications as to whether the service is still used or not, they are mere references. The service registry is the one that has that information. Plugins are keeping these ids so that if they are independently deactivated, they can ask the service registry to unregister its service offers. If this understanding holds, it would make sense for a plugin to expect and handle the exception when attempt to unregister a service fails because it is already gone.

[aaronayres35]

Ah, this would present a mental shift in understanding... The service ids on the plugins provide no indications as to whether the service is still used or not, they are mere references. The service registry is the one that has that information. Plugins are keeping these ids so that if they are independently deactivated, they can ask the service registry to unregister its service offers. If this understanding holds, it would make sense for a plugin to expect and handle the exception when attempt to unregister a service fails because it is already gone.

I see, maybe we could have a quick chat about this either before, after or during the standup today? Just to make sure my understanding is on the same page.
Trying to follow this suggestion, in the plugin.unregister_services method I simply made the following change:

    def unregister_services(self):
        """ Unregister any service offered by the plugin. """

        # Unregister the services in the reverse order that we registered
        # them.
        service_ids = self._service_ids[:]
        service_ids.reverse()

        for service_id in service_ids:
            try:
                self.application.unregister_service(service_id)
            except ValueError:
                pass

        # Just in case the plugin is started again!
        self._service_ids = []

        return

and I removed my listener in CorePlugin. With this change, all the tests pass. Basically if we see a ValueError when unregistering a service as a plugin we basically just assume that that service was already unregistered (by the service_registry). This is a bit ugly and there is probably a nicer way to do this, it was more just a sanity check of the different approach.

[kitchoi]

Some minor comments and questions.
(The test looks great, they are not junk 😂 .)

[kitchoi]

LGTM