refactor: review fixes for OSGi (#9289)

* refactoring: review fixes

* chore: add a comment about Jar packaging in test-root-context module

* fix: ensureServletContext should be always called

* fix: fix review comments
# Conflicts:
#	flow-tests/test-root-context/pom-npm.xml

flow-server/pom.xml

@@ -164,6 +164,7 @@
             <version>${osgi.core.version}</version>
             <scope>provided</scope>
         </dependency>
+        <!-- There is a registered  OSGi service in the module -->
         <dependency>
             <groupId>org.osgi</groupId>
             <artifactId>osgi.cmpn</artifactId>

flow-server/src/main/java/com/vaadin/flow/di/Lookup.java

@@ -57,16 +57,17 @@
 public interface Lookup {
 
     /**
-     * Lookup for a service by the provided {@code serviceClass}.
+     * Lookup for a service of the given type.
      * <p>
-     * The {@code serviceClass} is usually an interface class (though it doesn't
-     * have to be) and the returned value is some implementation of this
-     * interface.
+     * The {@code serviceClass} is usually an interface (though it doesn't have
+     * to be) and the returned value is some implementation of this interface.
      * 
      * @param <T>
      *            a service type
      * @param serviceClass
      *            a service SPI class
+     * 
+     * @see Lookup#lookupAll(Class)
      * @return a service which implements the {@code serviceClass}, may be
      *         {@code null} if no services are registered for this SPI
      */

flow-server/src/main/java/com/vaadin/flow/server/VaadinServletContext.java

@@ -54,11 +54,11 @@ public ServletContext getContext() {
      * Ensures there is a valid instance of {@link ServletContext}.
      */
     private void ensureServletContext() {
-        if (context == null
+        if (getContext() == null
                 && VaadinService.getCurrent() instanceof VaadinServletService) {
             context = ((VaadinServletService) VaadinService.getCurrent())
                     .getServlet().getServletContext();
-        } else if (context == null) {
+        } else if (getContext() == null) {
             throw new IllegalStateException(
                     "The underlying ServletContext of VaadinServletContext is null and there is no VaadinServletService to obtain it from.");
         }
@@ -67,11 +67,11 @@ private void ensureServletContext() {
     @Override
     public <T> T getAttribute(Class<T> type, Supplier<T> defaultValueSupplier) {
         ensureServletContext();
-        synchronized (this) {
-            Object result = context.getAttribute(type.getName());
+        synchronized (getContext()) {
+            Object result = getContext().getAttribute(type.getName());
             if (result == null && defaultValueSupplier != null) {
                 result = defaultValueSupplier.get();
-                context.setAttribute(type.getName(), result);
+                getContext().setAttribute(type.getName(), result);
             }
             return type.cast(result);
         }
@@ -82,40 +82,41 @@ public <T> void setAttribute(Class<T> clazz, T value) {
         if (value == null) {
             removeAttribute(clazz);
         } else {
-            synchronized (this) {
-                checkType(clazz);
-                context.setAttribute(clazz.getName(), value);
+            synchronized (getContext()) {
+                checkLookupDuplicate(clazz);
+                getContext().setAttribute(clazz.getName(), value);
             }
         }
     }
 
     @Override
     public void removeAttribute(Class<?> clazz) {
-        synchronized (this) {
-            checkType(clazz);
-            context.removeAttribute(clazz.getName());
+        synchronized (getContext()) {
+            checkLookupDuplicate(clazz);
+            getContext().removeAttribute(clazz.getName());
         }
     }
 
     @Override
     public Enumeration<String> getContextParameterNames() {
         ensureServletContext();
-        return context.getInitParameterNames();
+        return getContext().getInitParameterNames();
     }
 
     @Override
     public String getContextParameter(String name) {
         ensureServletContext();
-        return context.getInitParameter(name);
+        return getContext().getInitParameter(name);
     }
 
     private Object doGetAttribute(Class<?> clazz) {
         ensureServletContext();
-        return context.getAttribute(clazz.getName());
+        return getContext().getAttribute(clazz.getName());
     }
 
-    private void checkType(Class<?> type) {
-        if (Lookup.class.equals(type) && doGetAttribute(type) != null) {
+    private void checkLookupDuplicate(Class<?> type) {
+        Object attribute = doGetAttribute(type);
+        if (attribute != null && Lookup.class.equals(type)) {
             throw new IllegalArgumentException("The attribute " + Lookup.class
                     + " has been already set once. It's not possible to everride its value");
         }

flow-server/src/main/java/com/vaadin/flow/server/osgi/OSGiAccess.java

@@ -106,7 +106,7 @@ public <T> T lookup(Class<T> serviceClass) {
 
         @Override
         public <T> Collection<T> lookupAll(Class<T> serviceClass) {
-            Bundle bundle = FrameworkUtil.getBundle(LookupInitializer.class);
+            Bundle bundle = FrameworkUtil.getBundle(OSGiAccess.class);
             try {
                 Collection<ServiceReference<T>> references = bundle
                         .getBundleContext()
@@ -120,10 +120,10 @@ public <T> Collection<T> lookupAll(Class<T> serviceClass) {
                 }
                 return services;
             } catch (InvalidSyntaxException e) {
-                LoggerFactory.getLogger(OsgiLookupImpl.class).error(
-                        "Unexpected exception which is not expected to be thrown",
-                        e);
-                assert false : "Serveice filter is null so this may not happen";
+                LoggerFactory.getLogger(OsgiLookupImpl.class)
+                        .error("Unexpected invalid filter expression", e);
+                assert false : "Implementation error: Unexpected invalid filter exception is "
+                        + "thrown even though the service filter is null. Check the exception and update the impl";
             }
 
             return Collections.emptyList();

flow-server/src/main/java/com/vaadin/flow/server/osgi/VaadinBundleTracker.java

@@ -67,6 +67,9 @@ public class VaadinBundleTracker extends BundleTracker<Bundle> {
 
     private final AtomicReference<ServiceRegistration<Servlet>> servletRegistration = new AtomicReference<>();
 
+    /**
+     * Dedicated servlet for serving resources in Flow bundles.
+     */
     private static class PushResourceServlet extends HttpServlet {
 
         private final Bundle bundle;

flow-server/src/main/java/com/vaadin/flow/server/startup/ClassLoaderAwareServletContainerInitializer.java

@@ -48,6 +48,7 @@ public interface ClassLoaderAwareServletContainerInitializer
     @Override
     default void onStartup(Set<Class<?>> set, ServletContext context)
             throws ServletException {
+        // see DeferredServletContextIntializers
         DeferredServletContextInitializers.Initializer deferredInitializer = ctx -> {
             ClassLoader webClassLoader = ctx.getClassLoader();
             ClassLoader classLoader = getClass().getClassLoader();

flow-server/src/main/java/com/vaadin/flow/server/startup/LookupInitializer.java

@@ -61,22 +61,23 @@ public class LookupInitializer
 
     private static final String SPI = " SPI: ";
 
-    private static final String ONE_IMPL_REQUIRED = ". Only one implementation should be registered";
+    private static final String ONE_IMPL_REQUIRED = ". Only one implementation should be registered. "
+            + "Use lookupAll to get all instances of the given type.";
 
     private static final String SEVERAL_IMPLS = "Found several implementations in the classpath for ";
 
     private static class LookupImpl implements Lookup {
 
-        private final Map<Class<?>, Collection<Object>> services;
+        private final Map<Class<?>, Collection<Object>> serviceMap;
 
         private LookupImpl(Map<Class<?>, Collection<Object>> initialServices) {
-            services = Collections
+            serviceMap = Collections
                     .unmodifiableMap(new HashMap<>(initialServices));
         }
 
         @Override
         public <T> T lookup(Class<T> serviceClass) {
-            Collection<Object> registered = services.get(serviceClass);
+            Collection<Object> registered = serviceMap.get(serviceClass);
             if (registered == null || registered.isEmpty()) {
                 ServiceLoader<T> loader = ServiceLoader.load(serviceClass);
                 List<T> services = new ArrayList<>();
@@ -102,7 +103,7 @@ public <T> T lookup(Class<T> serviceClass) {
         @Override
         public <T> Collection<T> lookupAll(Class<T> serviceClass) {
             List<T> result = new ArrayList<>();
-            Collection<Object> registered = services.get(serviceClass);
+            Collection<Object> registered = serviceMap.get(serviceClass);
 
             Set<?> registeredClasses = registered == null
                     ? Collections.emptySet()
@@ -181,6 +182,10 @@ public URL getClientResource(String path) {
         @Override
         public InputStream getClientResourceAsStream(String path)
                 throws IOException {
+            // the client resource should be available in the classpath, so
+            // its content is cached once. If an exception is thrown then
+            // something is broken and it's also cached and will be rethrown on
+            // every subsequent access
             CachedStreamData cached = cache.computeIfAbsent(path, key -> {
                 URL url = getClientResource(key);
                 try (InputStream stream = url.openStream()) {