Init plugins' AllRequestFilters, even if they are loaded after startup

When installing a plugin with AllRequestFilters after Gerrit has been
started up, the filter's doFilter is called as expected. But the
filter's init method is never called, as FilterProxy did not keep
track of which filters have been around when its own init got called.

This lack of calling init, breaks the Filter contract, and makes the
javamelody plugin throw NPEs [1] for each request when installing it
in a running Gerrit. While restarting Gerrit renders the javamelody
plugin working, the root problem is the missing call to the
filter's init.

Hence, we make FilterProxy call init for plugins that did not get
initialized when FilterProxy itself got initialized. Thereby, the
javamelody plugin can be loaded directly into a running Gerrit again.

[1]
  [2015-08-03 23:20:01,379] WARN  org.eclipse.jetty.servlet.ServletHandler : /
  java.lang.NullPointerException
          at net.bull.javamelody.MonitoringFilter.doFilter(MonitoringFilter.java:170)
          at com.googlesource.gerrit.plugins.javamelody.GerritMonitoringFilter.doFilter(GerritMonitoringFilter.java:73)
  [...]

Change-Id: I095ef517210911c982438d8ce3a7740d05c27eee

gerrit-extension-api/BUCK

@@ -51,6 +51,17 @@ java_sources(
   visibility = ['PUBLIC'],
 )
 
+java_test(
+  name = 'api_tests',
+  srcs = glob(['src/test/java/**/*.java']),
+  deps = [
+    ':api',
+    '//lib:truth',
+    '//lib/guice:guice',
+  ],
+  source_under_test = [':api'],
+)
+
 java_doc(
   name = 'extension-api-javadoc',
   title = 'Gerrit Review Extension API Documentation',

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/registration/DynamicSet.java

@@ -181,6 +181,23 @@ public class DynamicSet<T> implements Iterable<T> {
     };
   }
 
+  /**
+   * Returns {@code true} if this set contains the given item.
+   *
+   * @param item item to check whether or not it is contained.
+   * @return {@code true} if this set contains the given item.
+   */
+  public boolean contains(final T item) {
+    Iterator<T> iterator = iterator();
+    while (iterator.hasNext()) {
+      T candidate = iterator.next();
+      if (candidate == item) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   /**
    * Add one new element to the set.
    *

gerrit-extension-api/src/test/java/com/google/gerrit/extensions/registration/DynamicSetTest.java

@@ -0,0 +1,85 @@
+// Copyright (C) 2015 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.extensions.registration;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.inject.Key;
+import com.google.inject.util.Providers;
+
+import org.junit.Test;
+
+public class DynamicSetTest {
+  @Test
+  public void testContainsWithEmpty() throws Exception {
+    DynamicSet<Integer> ds = new DynamicSet<>();
+    assertThat(ds).doesNotContain(2);
+  }
+
+  @Test
+  public void testContainsTrueWithSingleElement() throws Exception {
+    DynamicSet<Integer> ds = new DynamicSet<>();
+    ds.add(2);
+
+    assertThat(ds).contains(2);
+  }
+
+  @Test
+  public void testContainsFalseWithSingleElement() throws Exception {
+    DynamicSet<Integer> ds = new DynamicSet<>();
+    ds.add(2);
+
+    assertThat(ds).doesNotContain(3);
+  }
+
+  @Test
+  public void testContainsTrueWithTwoElements() throws Exception {
+    DynamicSet<Integer> ds = new DynamicSet<>();
+    ds.add(2);
+    ds.add(4);
+
+    assertThat(ds).contains(4);
+  }
+
+  @Test
+  public void testContainsFalseWithTwoElements() throws Exception {
+    DynamicSet<Integer> ds = new DynamicSet<>();
+    ds.add(2);
+    ds.add(4);
+
+    assertThat(ds).doesNotContain(3);
+  }
+
+  @Test
+  public void testContainsDynamic() throws Exception {
+    DynamicSet<Integer> ds = new DynamicSet<>();
+    ds.add(2);
+
+    Key<Integer> key = Key.get(Integer.class);
+    ReloadableRegistrationHandle<Integer> handle = ds.add(key, Providers.of(4));
+
+    ds.add(6);
+
+    // At first, 4 is contained.
+    assertThat(ds).contains(4);
+
+    // Then we remove 4.
+    handle.remove();
+
+    // And now 4 should no longer be contained.
+    assertThat(ds).doesNotContain(4);
+  }
+}

gerrit-httpd/src/main/java/com/google/gerrit/httpd/AllRequestFilter.java

@@ -15,8 +15,11 @@
 package com.google.gerrit.httpd;
 
 import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.plugins.Plugin;
+import com.google.gerrit.server.plugins.StopPluginListener;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
+import com.google.inject.internal.UniqueAnnotations;
 import com.google.inject.servlet.ServletModule;
 
 import java.io.IOException;
@@ -37,17 +40,62 @@ public abstract class AllRequestFilter implements Filter {
       protected void configureServlets() {
         DynamicSet.setOf(binder(), AllRequestFilter.class);
         filter("/*").through(FilterProxy.class);
+
+        bind(StopPluginListener.class)
+          .annotatedWith(UniqueAnnotations.create())
+          .to(FilterProxy.class);
       }
     };
   }
 
   @Singleton
-  static class FilterProxy implements Filter {
+  static class FilterProxy implements Filter, StopPluginListener {
     private final DynamicSet<AllRequestFilter> filters;
 
+    private DynamicSet<AllRequestFilter> initializedFilters;
+    private FilterConfig filterConfig;
+
     @Inject
     FilterProxy(DynamicSet<AllRequestFilter> filters) {
       this.filters = filters;
+      this.initializedFilters = new DynamicSet<>();
+      this.filterConfig = null;
+    }
+
+    /**
+     * Initializes a filter if needed
+     *
+     * @param filter The filter that should get initialized
+     * @return {@code true} iff filter is now initialized
+     * @throws ServletException if filter itself fails to init
+     */
+    private synchronized boolean initFilterIfNeeded(AllRequestFilter filter)
+        throws ServletException {
+      boolean ret = true;
+      if (filters.contains(filter)) {
+        // Regardless of whether or not the caller checked filter's
+        // containment in initializedFilters, we better re-check as we're now
+        // synchronized.
+        if (!initializedFilters.contains(filter)) {
+          filter.init(filterConfig);
+          initializedFilters.add(filter);
+        }
+      } else {
+        ret = false;
+      }
+      return ret;
+    }
+
+    private synchronized void cleanUpInitializedFilters() {
+      Iterable<AllRequestFilter> filtersToCleanUp  = initializedFilters;
+      initializedFilters = new DynamicSet<>();
+      for (AllRequestFilter filter : filtersToCleanUp) {
+        if (filters.contains(filter)) {
+          initializedFilters.add(filter);
+        } else {
+          filter.destroy();
+        }
+      }
     }
 
     @Override
@@ -58,28 +106,67 @@ public abstract class AllRequestFilter implements Filter {
         @Override
         public void doFilter(ServletRequest req, ServletResponse res)
             throws IOException, ServletException {
-          if (itr.hasNext()) {
-            itr.next().doFilter(req, res, this);
-          } else {
-            last.doFilter(req, res);
+          while (itr.hasNext()) {
+            AllRequestFilter filter = itr.next();
+            // To avoid {@code synchronized} on the the whole filtering (and
+            // thereby killing concurrency), we start the below disjunction
+            // with an unsynchronized check for containment. This
+            // unsynchronized check is always correct if no filters got
+            // initialized/cleaned concurrently behind our back.
+            // The case of concurrently initialized filters is saved by the
+            // call to initFilterIfNeeded. So that's fine too.
+            // The case of concurrently cleaned filters between the {@code if}
+            // condition and the call to {@code doFilter} is not saved by
+            // anything. If a filter is getting removed concurrently while
+            // another thread is in those two lines, doFilter might (but need
+            // not) fail.
+            //
+            // Since this failure only occurs if a filter is deleted
+            // (e.g.: a plugin reloaded) exactly when a thread is in those
+            // two lines, and it only breaks a single request, we're ok with
+            // it, given that this is really both really improbable and also
+            // the "proper" fix for it would basically kill concurrency of
+            // webrequests.
+            if (initializedFilters.contains(filter)
+                || initFilterIfNeeded(filter)) {
+              filter.doFilter(req, res, this);
+              return;
+            }
           }
+          last.doFilter(req, res);
         }
       }.doFilter(req, res);
     }
 
     @Override
     public void init(FilterConfig config) throws ServletException {
+      // Plugins that provide AllRequestFilters might get loaded later at
+      // runtime, long after this init method had been called. To allow to
+      // correctly init such plugins' AllRequestFilters, we keep the
+      // FilterConfig around, and reuse it to lazy init the AllRequestFilters.
+      filterConfig = config;
+
       for (AllRequestFilter f: filters) {
-        f.init(config);
+        initFilterIfNeeded(f);
       }
     }
 
     @Override
-    public void destroy() {
-      for (AllRequestFilter f: filters) {
-        f.destroy();
+    public synchronized void destroy() {
+      Iterable<AllRequestFilter> filtersToDestroy  = initializedFilters;
+      initializedFilters = new DynamicSet<>();
+      for (AllRequestFilter filter: filtersToDestroy) {
+        filter.destroy();
       }
     }
+
+    @Override
+    public void onStopPlugin(Plugin plugin) {
+      // In order to allow properly garbage collection, we need to scrub
+      // initializedFilters clean of filters stemming from plugins as they
+      // get unloaded.
+      cleanUpInitializedFilters();
+    }
   }
 
   @Override

gerrit-httpd/src/test/java/com/google/gerrit/httpd/AllRequestFilterFilterProxyTest.java

@@ -20,6 +20,10 @@ import static org.easymock.EasyMock.eq;
 import static org.easymock.EasyMock.newCapture;
 
 import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.registration.ReloadableRegistrationHandle;
+import com.google.gerrit.server.plugins.Plugin;
+import com.google.inject.Key;
+import com.google.inject.util.Providers;
 
 import org.easymock.Capture;
 import org.easymock.EasyMockSupport;
@@ -73,8 +77,10 @@ public class AllRequestFilterFilterProxyTest {
    * {@link AllRequestFilter.FilterProxy} instances created by
    * {@link #getFilterProxy()}.
    */
-  private void addFilter(final AllRequestFilter filter) {
-    filters.add(filter);
+  private ReloadableRegistrationHandle<AllRequestFilter> addFilter(
+      final AllRequestFilter filter) {
+    Key<AllRequestFilter> key = Key.get(AllRequestFilter.class);
+    return filters.add(key, Providers.of(filter));
   }
 
   @Test
@@ -230,4 +236,131 @@ public class AllRequestFilterFilterProxyTest {
 
     ems.verifyAll();
   }
+
+  @Test
+  public void testPostponedLoading() throws Exception {
+    EasyMockSupport ems = new EasyMockSupport();
+
+    FilterConfig config = ems.createMock(FilterConfig.class);
+    HttpServletRequest req1 = ems.createMock("req1", HttpServletRequest.class);
+    HttpServletRequest req2 = ems.createMock("req2", HttpServletRequest.class);
+    HttpServletResponse res1 = ems.createMock("res1", HttpServletResponse.class);
+    HttpServletResponse res2 = ems.createMock("res2", HttpServletResponse.class);
+
+    IMocksControl mockControl = ems.createStrictControl();
+    FilterChain chain = mockControl.createMock("chain", FilterChain.class);
+
+    Capture<FilterChain> capturedChainA1 = newCapture();
+    Capture<FilterChain> capturedChainA2 = newCapture();
+    Capture<FilterChain> capturedChainB = newCapture();
+
+    AllRequestFilter filterA = mockControl.createMock("filterA", AllRequestFilter.class);
+    AllRequestFilter filterB = mockControl.createMock("filterB", AllRequestFilter.class);
+
+    filterA.init(config);
+    filterA.doFilter(eq(req1), eq(res1), capture(capturedChainA1));
+    chain.doFilter(req1, res1);
+
+    filterA.doFilter(eq(req2), eq(res2), capture(capturedChainA2));
+    filterB.init(config); // <-- This is crucial part. filterB got loaded
+    // after filterProxy's init finished. Nonetheless filterB gets initialized.
+    filterB.doFilter(eq(req2), eq(res2), capture(capturedChainB));
+    chain.doFilter(req2, res2);
+
+    filterA.destroy();
+    filterB.destroy();
+
+    ems.replayAll();
+
+    AllRequestFilter.FilterProxy filterProxy = getFilterProxy();
+    addFilter(filterA);
+
+    filterProxy.init(config);
+    filterProxy.doFilter(req1, res1, chain);
+    capturedChainA1.getValue().doFilter(req1, res1);
+
+    addFilter(filterB); // <-- Adds filter after filterProxy's init got called.
+    filterProxy.doFilter(req2, res2, chain);
+    capturedChainA2.getValue().doFilter(req2, res2);
+    capturedChainB.getValue().doFilter(req2, res2);
+
+    filterProxy.destroy();
+
+    ems.verifyAll();
+  }
+
+  @Test
+  public void testDynamicUnloading() throws Exception {
+    EasyMockSupport ems = new EasyMockSupport();
+
+    FilterConfig config = ems.createMock(FilterConfig.class);
+    HttpServletRequest req1 = ems.createMock("req1", HttpServletRequest.class);
+    HttpServletRequest req2 = ems.createMock("req2", HttpServletRequest.class);
+    HttpServletRequest req3 = ems.createMock("req3", HttpServletRequest.class);
+    HttpServletResponse res1 = ems.createMock("res1", HttpServletResponse.class);
+    HttpServletResponse res2 = ems.createMock("res2", HttpServletResponse.class);
+    HttpServletResponse res3 = ems.createMock("res3", HttpServletResponse.class);
+
+    Plugin plugin = ems.createMock(Plugin.class);
+
+    IMocksControl mockControl = ems.createStrictControl();
+    FilterChain chain = mockControl.createMock("chain", FilterChain.class);
+
+    Capture<FilterChain> capturedChainA1 = newCapture();
+    Capture<FilterChain> capturedChainB1 = newCapture();
+    Capture<FilterChain> capturedChainB2 = newCapture();
+
+    AllRequestFilter filterA = mockControl.createMock("filterA", AllRequestFilter.class);
+    AllRequestFilter filterB = mockControl.createMock("filterB", AllRequestFilter.class);
+
+    filterA.init(config);
+    filterB.init(config);
+
+    filterA.doFilter(eq(req1), eq(res1), capture(capturedChainA1));
+    filterB.doFilter(eq(req1), eq(res1), capture(capturedChainB1));
+    chain.doFilter(req1, res1);
+
+    filterA.destroy(); // Cleaning up of filterA after it got unloaded
+
+    filterB.doFilter(eq(req2), eq(res2), capture(capturedChainB2));
+    chain.doFilter(req2, res2);
+
+    filterB.destroy(); // Cleaning up of filterA after it got unloaded
+
+    chain.doFilter(req3, res3);
+
+    ems.replayAll();
+
+    AllRequestFilter.FilterProxy filterProxy = getFilterProxy();
+    ReloadableRegistrationHandle<AllRequestFilter> handleFilterA =
+        addFilter(filterA);
+    ReloadableRegistrationHandle<AllRequestFilter> handleFilterB =
+        addFilter(filterB);
+
+    filterProxy.init(config);
+
+    // Request #1 with filterA and filterB
+    filterProxy.doFilter(req1, res1, chain);
+    capturedChainA1.getValue().doFilter(req1, res1);
+    capturedChainB1.getValue().doFilter(req1, res1);
+
+    // Unloading filterA
+    handleFilterA.remove();
+    filterProxy.onStopPlugin(plugin);
+
+    // Request #1 only with filterB
+    filterProxy.doFilter(req2, res2, chain);
+    capturedChainA1.getValue().doFilter(req2, res2);
+
+    // Unloading filterB
+    handleFilterB.remove();
+    filterProxy.onStopPlugin(plugin);
+
+    // Request #1 with no additional filters
+    filterProxy.doFilter(req3, res3, chain);
+
+    filterProxy.destroy();
+
+    ems.verifyAll();
+  }
 }