Fix Unregister service value error bug

envisage/core_plugin.py

@@ -138,7 +138,6 @@ def _service_offers_changed(self, event):
 
     def start(self):
         """ Start the plugin. """
-
         # Load all contributed preferences files into the application's root
         # preferences node.
         self._load_preferences(self.preferences)

envisage/plugin.py

@@ -289,7 +289,13 @@ def unregister_services(self):
         service_ids.reverse()
 
         for service_id in service_ids:
-            self.application.unregister_service(service_id)
+            # note the service may have already been individually unregistered
+            try:
+                self.application.service_registry.get_service_from_id(service_id)  # noqa: E501
+            except ValueError:
+                pass
+            else:
+                self.application.unregister_service(service_id)
 
         # Just in case the plugin is started again!
         self._service_ids = []

envisage/tests/test_core_plugin.py

@@ -16,9 +16,21 @@
 from pkg_resources import resource_filename
 
 # Enthought library imports.
-from envisage.api import Application, Plugin
+from envisage.api import Application, CorePlugin, Plugin
 from envisage.api import ServiceOffer
-from traits.api import Interface, List
+from traits.api import (
+    HasTraits,
+    Interface,
+    List,
+    on_trait_change,
+    pop_exception_handler as pop_on_trait_change_handler,
+    push_exception_handler as push_on_trait_change_handler,
+    Str,
+)
+from traits.observation.api import (
+    pop_exception_handler as pop_observe_handler,
+    push_exception_handler as push_observe_handler,
+)
 
 
 # This module's package.
@@ -34,11 +46,17 @@ class TestApplication(Application):
 class CorePluginTestCase(unittest.TestCase):
     """ Tests for the core plugin. """
 
+    def setUp(self):
+        push_on_trait_change_handler(reraise_exceptions=True)
+        push_observe_handler(reraise_exceptions=True)
+
+    def tearDown(self):
+        pop_observe_handler()
+        pop_on_trait_change_handler()
+
     def test_service_offers(self):
         """ service offers """
 
-        from envisage.api import CorePlugin
-
         class IMyService(Interface):
             pass
 
@@ -81,8 +99,6 @@ def _my_service_factory(self, **properties):
     def test_dynamically_added_service_offer(self):
         """ dynamically added service offer """
 
-        from envisage.api import CorePlugin
-
         class IMyService(Interface):
             pass
 
@@ -137,10 +153,6 @@ def _my_service_factory(self, **properties):
     def test_preferences(self):
         """ preferences """
 
-        # The core plugin is the plugin that offers the preferences extension
-        # point.
-        from envisage.api import CorePlugin
-
         class PluginA(Plugin):
             id = "A"
             preferences = List(contributes_to="envisage.preferences")
@@ -162,10 +174,6 @@ def _preferences_default(self):
     def test_dynamically_added_preferences(self):
         """ dynamically added preferences """
 
-        # The core plugin is the plugin that offers the preferences extension
-        # point.
-        from envisage.api import CorePlugin
-
         class PluginA(Plugin):
             id = "A"
             preferences = List(contributes_to="envisage.preferences")
@@ -187,3 +195,65 @@ def _preferences_default(self):
 
         # Make sure we can get one of the preferences.
         self.assertEqual("42", application.preferences.get("enthought.test.x"))
+
+    # regression test for enthought/envisage#251
+    def test_unregister_service_offer(self):
+
+        class IJunk(Interface):
+            trash = Str()
+
+        class Junk(HasTraits):
+            trash = Str("garbage")
+
+        class PluginA(Plugin):
+            # The Ids of the extension points that this plugin contributes to.
+            SERVICE_OFFERS = "envisage.service_offers"
+
+            service_offers = List(contributes_to=SERVICE_OFFERS)
+
+            def _service_offers_default(self):
+
+                a_service_offer = ServiceOffer(
+                    protocol=IJunk,
+                    factory=self._create_junk_service,
+                )
+
+                return [a_service_offer]
+
+            def _create_junk_service(self):
+                """ Factory method for the 'Junk' service. """
+
+                return Junk()
+
+            @on_trait_change("application:started")
+            def _unregister_junk_service(self):
+                # only 1 service is registered so it has service_id of 1
+                self.application.unregister_service(1)
+
+        application = TestApplication(
+            plugins=[CorePlugin(), PluginA()],
+        )
+
+        # Run it!
+        return application.run()
+
+    def test_unregister_service(self):
+
+        class IJunk(Interface):
+            trash = Str()
+
+        class Junk(HasTraits):
+            trash = Str("garbage")
+
+        some_junk = Junk()
+
+        application = TestApplication(
+            plugins=[CorePlugin()],
+        )
+
+        application.start()
+
+        some_junk_id = application.register_service(IJunk, some_junk)
+        application.unregister_service(some_junk_id)
+
+        application.stop()