From a108a7e8da24d5f38e5e3f34e4a0e95c1cb6b56e Mon Sep 17 00:00:00 2001
From: Tim Ward <timothyjward@apache.org>
Date: Wed, 19 Jul 2017 07:17:46 +0100
Subject: Tidy up the Vaadin OSGi whiteboard component (#9648)

The Vaadin OSGi integration component uses Declarative Services, but it does some odd things:
 * It uses the whiteboard service's own bundle context to get hold of the service instance
 * It has an asymmetric get/release for the whiteboard service, which could leak instances over time
 * It releases service instances that it is still actively using

This change tidies up the service lifecycle by delegating the get/release to the Service Component Runtime managing the container (specifically by injecting the service instance). Using this injection also ensures that the Vaadin whiteboard service is obtained using this component's bundle context.

This change also simplifies the code a little by using the reference as the key to track the registrations. Different references for the same service are required to be equal so there is no issue with doing this.

This change does not alter the fact that the whiteboard service's bundle context is used to register the Http Whiteboard servlet, as this may be the intended behaviour. As a result the component should be prepared for an IllegalStateException when unregistering the service whiteboard service, which may already have been unregistered by the OSGi framework if the target bundle is stopping.
---
 .../osgi/servlet/VaadinServletRegistration.java    | 39 +++++++++++-----------
 1 file changed, 20 insertions(+), 19 deletions(-)

(limited to 'osgi-integration/src/main/java/com/vaadin')

diff --git a/osgi-integration/src/main/java/com/vaadin/osgi/servlet/VaadinServletRegistration.java b/osgi-integration/src/main/java/com/vaadin/osgi/servlet/VaadinServletRegistration.java
index d7ec6289ce..4ab1cf1505 100644
--- a/osgi-integration/src/main/java/com/vaadin/osgi/servlet/VaadinServletRegistration.java
+++ b/osgi-integration/src/main/java/com/vaadin/osgi/servlet/VaadinServletRegistration.java
@@ -48,9 +48,9 @@ import com.vaadin.server.VaadinServlet;
  *
  * @since 8.1
  */
-@Component(immediate = true)
+@Component
 public class VaadinServletRegistration {
-    private Map<Long, ServiceRegistration<?>> registeredServlets = Collections
+    private final Map<ServiceReference<VaadinServlet>, ServiceRegistration<Servlet>> registeredServlets = Collections
             .synchronizedMap(new LinkedHashMap<>());
 
     private static final String MISSING_ANNOTATION_MESSAGE_FORMAT = "The property '%s' must be set in a '%s' without the '%s' annotation!";
@@ -64,19 +64,18 @@ public class VaadinServletRegistration {
     private LogService logService;
 
     @Reference(cardinality = ReferenceCardinality.MULTIPLE, service = VaadinServlet.class, policy = ReferencePolicy.DYNAMIC)
-    void bindVaadinServlet(ServiceReference<VaadinServlet> reference)
+    void bindVaadinServlet(VaadinServlet servlet, ServiceReference<VaadinServlet> reference)
             throws ResourceBundleInactiveException {
         log(LogService.LOG_WARNING, "VaadinServlet Registration");
-        BundleContext bundleContext = reference.getBundle().getBundleContext();
-        Hashtable<String, Object> properties = getProperties(reference);
 
-        VaadinServlet servlet = bundleContext.getService(reference);
+        Hashtable<String, Object> properties = getProperties(reference);
 
         WebServlet annotation = servlet.getClass()
                 .getAnnotation(WebServlet.class);
 
-        if (!validateSettings(annotation, properties))
+        if (!validateSettings(annotation, properties)) {
             return;
+        }
 
         properties.put(VAADIN_RESOURCES_PARAM, getResourcePath());
         if (annotation != null) {
@@ -85,17 +84,13 @@ public class VaadinServletRegistration {
                     Boolean.toString(annotation.asyncSupported()));
         }
 
+        // We register the Http Whiteboard servlet using the context of
+        // the bundle which registered the Vaadin Servlet, not our own
+        BundleContext bundleContext = reference.getBundle().getBundleContext();
         ServiceRegistration<Servlet> servletRegistration = bundleContext
                 .registerService(Servlet.class, servlet, properties);
-        Long serviceId = getServiceId(reference);
-        registeredServlets.put(serviceId, servletRegistration);
-
-        bundleContext.ungetService(reference);
-    }
 
-    private Long getServiceId(ServiceReference<VaadinServlet> reference) {
-        return (Long) reference
-                .getProperty(org.osgi.framework.Constants.SERVICE_ID);
+        registeredServlets.put(reference, servletRegistration);
     }
 
     private boolean validateSettings(WebServlet annotation,
@@ -128,12 +123,18 @@ public class VaadinServletRegistration {
         }
     }
 
-    void unbindVaadinServlet(ServiceReference<VaadinServlet> servletRef) {
-        Long serviceId = getServiceId(servletRef);
+    void unbindVaadinServlet(ServiceReference<VaadinServlet> reference) {
         ServiceRegistration<?> servletRegistration = registeredServlets
-                .remove(serviceId);
+                .remove(reference);
         if (servletRegistration != null) {
-            servletRegistration.unregister();
+            try {
+                servletRegistration.unregister();
+            } catch (IllegalStateException ise) {
+                // This service may have already been unregistered
+                // automatically by the OSGi framework if the
+                // application bundle is being stopped. This is
+                // obviously not a problem for us.
+            }
         }
     }
 
-- 
cgit v1.2.3