fix: Initialize VaadinServlet after Lookup is available (#9500)

fix: don't initialize Vaadin servlet until Lookup is not available
vaadin · Dec 7, 2020 · 28071d9 · 28071d9
1 parent 8077c26
commit 28071d9
Show file tree

Hide file tree

Showing 6 changed files with 181 additions and 36 deletions.
diff --git a/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java b/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java
@@ -15,13 +15,13 @@
  */
 package com.vaadin.flow.server;
 
-import java.util.Collections;
-import java.util.List;
-
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletContext;
 import javax.servlet.ServletException;
 
+import java.util.Collections;
+import java.util.List;
+
 import org.mockito.Mockito;
 
 import com.vaadin.flow.di.Instantiator;
@@ -112,15 +112,18 @@ public void init() {
         try {
             MockVaadinServlet servlet = (MockVaadinServlet) getServlet();
             servlet.service = this;
-            ServletConfig config = Mockito.mock(ServletConfig.class);
-            ServletContext context = Mockito.mock(ServletContext.class);
-            Mockito.when(config.getServletContext()).thenReturn(context);
-
-            Mockito.when(lookup.lookup(ResourceProvider.class))
-                    .thenReturn(resourceProvider);
-            Mockito.when(context.getAttribute(Lookup.class.getName()))
-                    .thenReturn(lookup);
-            getServlet().init(config);
+
+            if (getServlet().getServletConfig() == null) {
+                ServletConfig config = Mockito.mock(ServletConfig.class);
+                ServletContext context = Mockito.mock(ServletContext.class);
+                Mockito.when(config.getServletContext()).thenReturn(context);
+
+                Mockito.when(lookup.lookup(ResourceProvider.class))
+                        .thenReturn(resourceProvider);
+                Mockito.when(context.getAttribute(Lookup.class.getName()))
+                        .thenReturn(lookup);
+                getServlet().init(config);
+            }
             super.init();
         } catch (ServiceException | ServletException e) {
             throw new RuntimeException(e);

diff --git a/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java b/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java
@@ -15,13 +15,13 @@
  */
 package com.vaadin.flow.server;
 
-import java.util.Collections;
-import java.util.List;
-
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletContext;
 import javax.servlet.ServletException;
 
+import java.util.Collections;
+import java.util.List;
+
 import org.mockito.Mockito;
 
 import com.vaadin.flow.di.Instantiator;
@@ -112,15 +112,18 @@ public void init() {
         try {
             MockVaadinServlet servlet = (MockVaadinServlet) getServlet();
             servlet.service = this;
-            ServletConfig config = Mockito.mock(ServletConfig.class);
-            ServletContext context = Mockito.mock(ServletContext.class);
-            Mockito.when(config.getServletContext()).thenReturn(context);
-
-            Mockito.when(lookup.lookup(ResourceProvider.class))
-                    .thenReturn(resourceProvider);
-            Mockito.when(context.getAttribute(Lookup.class.getName()))
-                    .thenReturn(lookup);
-            getServlet().init(config);
+
+            if (getServlet().getServletConfig() == null) {
+                ServletConfig config = Mockito.mock(ServletConfig.class);
+                ServletContext context = Mockito.mock(ServletContext.class);
+                Mockito.when(config.getServletContext()).thenReturn(context);
+
+                Mockito.when(lookup.lookup(ResourceProvider.class))
+                        .thenReturn(resourceProvider);
+                Mockito.when(context.getAttribute(Lookup.class.getName()))
+                        .thenReturn(lookup);
+                getServlet().init(config);
+            }
             super.init();
         } catch (ServiceException | ServletException e) {
             throw new RuntimeException(e);

diff --git a/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java b/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java
@@ -16,6 +16,7 @@
 package com.vaadin.flow.server;
 
 import javax.servlet.ServletConfig;
+import javax.servlet.ServletContext;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
@@ -27,6 +28,7 @@
 import java.util.Properties;
 
 import com.vaadin.flow.component.UI;
+import com.vaadin.flow.di.Lookup;
 import com.vaadin.flow.function.DeploymentConfiguration;
 import com.vaadin.flow.internal.CurrentInstance;
 import com.vaadin.flow.server.HandlerHelper.RequestType;
@@ -64,23 +66,59 @@ public class VaadinServlet extends HttpServlet {
     @Override
     public void init(ServletConfig servletConfig) throws ServletException {
         CurrentInstance.clearAll();
-        super.init(servletConfig);
 
         try {
-            servletService = createServletService();
-        } catch (ServiceException e) {
-            throw new ServletException("Could not initialize VaadinServlet", e);
-        }
+            /*
+             * There are plenty of reasons why the check should be done. The
+             * main reason is: init method is public which means that everyone
+             * may call this method at any time (including an app developer).
+             * But it's not supposed to be called any times any time.
+             * 
+             * This code protects weak API from being called several times so
+             * that config is reset after the very first initialization.
+             * 
+             * Normally "init" method is called only once by the servlet
+             * container. But in a specific OSGi case {@code
+             * ServletCointextListener} may be called after the servlet
+             * initialized. To be able to initialize the VaadinServlet properly
+             * its "init" method is called from the {@code
+             * ServletCointextListener} with the same ServletConfig instance.
+             */
+            if (getServletConfig() == null) {
+                super.init(servletConfig);
+            }
+
+            if (getServletConfig() != servletConfig) {
+                throw new IllegalArgumentException(
+                        "Servlet config instance may not differ from the "
+                                + "instance which has been used for the initial method call");
+            }
+
+            ServletContext servletContext = getServletConfig()
+                    .getServletContext();
+            if (servletService != null
+                    || new VaadinServletContext(servletContext)
+                            .getAttribute(Lookup.class) == null) {
+                return;
+            }
 
-        // Sets current service as it is needed in static file server even
-        // though there are no request and response.
-        servletService.setCurrentInstances(null, null);
+            try {
+                servletService = createServletService();
+            } catch (ServiceException e) {
+                throw new ServletException("Could not initialize VaadinServlet",
+                        e);
+            }
 
-        staticFileHandler = createStaticFileHandler(servletService);
+            // Sets current service as it is needed in static file server even
+            // though there are no request and response.
+            servletService.setCurrentInstances(null, null);
 
-        servletInitialized();
-        CurrentInstance.clearAll();
+            staticFileHandler = createStaticFileHandler(servletService);
 
+            servletInitialized();
+        } finally {
+            CurrentInstance.clearAll();
+        }
     }
 
     /**

diff --git a/flow-server/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java b/flow-server/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java
@@ -109,7 +109,9 @@ public void init() {
         try {
             MockVaadinServlet servlet = (MockVaadinServlet) getServlet();
             servlet.service = this;
-            getServlet().init(new MockServletConfig());
+            if (getServlet().getServletConfig() == null) {
+                getServlet().init(new MockServletConfig());
+            }
             super.init();
         } catch (ServiceException | ServletException e) {
             throw new RuntimeException(e);

diff --git a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java
@@ -212,6 +212,8 @@ public void nullUrlIsSetToACriticalNotification() {
     public void serviceContainsStreamRequestHandler()
             throws ServiceException, ServletException {
         ServletConfig servletConfig = new MockServletConfig();
+        servletConfig.getServletContext().setAttribute(Lookup.class.getName(),
+                Mockito.mock(Lookup.class));
         VaadinServlet servlet = new VaadinServlet() {
             @Override
             protected DeploymentConfiguration createDeploymentConfiguration()

diff --git a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java
@@ -15,14 +15,20 @@
  */
 package com.vaadin.flow.server;
 
+import javax.servlet.ServletConfig;
 import javax.servlet.ServletContext;
+import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 
+import java.util.concurrent.atomic.AtomicBoolean;
+
 import net.jcip.annotations.NotThreadSafe;
 import org.junit.Assert;
 import org.junit.Test;
 import org.mockito.Mockito;
 
+import com.vaadin.flow.di.Lookup;
+
 @NotThreadSafe
 public class VaadinServletTest {
 
@@ -64,6 +70,97 @@ public void testGetLastPathParameter() {
                 .getLastPathParameter("http://myhost.com/a;hello/;b=1,c=2/"));
     }
 
+    @Test
+    public void init_superInitCalledOnce() throws ServletException {
+        AtomicBoolean called = new AtomicBoolean();
+        VaadinServlet servlet = new VaadinServlet() {
+
+            @Override
+            public void init() throws ServletException {
+                Assert.assertFalse(called.get());
+                called.set(true);
+            }
+        };
+
+        ServletConfig config = mockConfig();
+        servlet.init(config);
+
+        Assert.assertTrue(called.get());
+
+        servlet.init(config);
+
+        Assert.assertSame(config, servlet.getServletConfig());
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void init_passDifferentConfigInstance_throws()
+            throws ServletException {
+        VaadinServlet servlet = new VaadinServlet();
+
+        ServletConfig config = mockConfig();
+        servlet.init(config);
+
+        servlet.init(mockConfig());
+    }
+
+    @Test
+    public void init_noLookup_servletIsNotInitialized()
+            throws ServletException {
+        AtomicBoolean called = new AtomicBoolean();
+        VaadinServlet servlet = new VaadinServlet() {
+
+            @Override
+            protected void servletInitialized() throws ServletException {
+                called.set(true);
+            }
+        };
+
+        ServletConfig config = mockConfig();
+        servlet.init(config);
+
+        Assert.assertFalse(called.get());
+    }
+
+    @Test
+    public void init_contextHasLookup_servletIsInitialized()
+            throws ServletException {
+        AtomicBoolean called = new AtomicBoolean();
+        VaadinServlet servlet = new VaadinServlet() {
+
+            @Override
+            protected VaadinServletService createServletService()
+                    throws ServletException, ServiceException {
+                return Mockito.mock(VaadinServletService.class);
+            }
+
+            @Override
+            protected StaticFileHandler createStaticFileHandler(
+                    VaadinServletService servletService) {
+                return Mockito.mock(StaticFileHandler.class);
+            }
+
+            @Override
+            protected void servletInitialized() throws ServletException {
+                called.set(true);
+            }
+        };
+
+        ServletConfig config = mockConfig();
+        ServletContext servletContext = config.getServletContext();
+        Mockito.when(servletContext.getAttribute(Lookup.class.getName()))
+                .thenReturn(Mockito.mock(Lookup.class));
+        servlet.init(config);
+
+        Assert.assertTrue(called.get());
+    }
+
+    private ServletConfig mockConfig() {
+        ServletConfig config = Mockito.mock(ServletConfig.class);
+        ServletContext context = Mockito.mock(ServletContext.class);
+        Mockito.when(config.getServletContext()).thenReturn(context);
+        return config;
+    }
+
     private HttpServletRequest createRequest(
             MockServletServiceSessionSetup mocks, String path) {
         HttpServletRequest httpServletRequest = Mockito