ensuring thread safe framework reload

Author: tulinkry

src/org/opensolaris/opengrok/authorization/AuthorizationEntity.java

@@ -51,7 +51,7 @@
  *
  * @author Krystof Tulinger
  */
-public abstract class AuthorizationEntity implements Nameable, Serializable {
+public abstract class AuthorizationEntity implements Nameable, Serializable, Cloneable {
 
     private static final long serialVersionUID = 1L;
     /**
@@ -63,6 +63,32 @@ public abstract class AuthorizationEntity implements Nameable, Serializable {
 
     protected transient boolean working = true;
 
+    public AuthorizationEntity() {
+    }
+
+    /**
+     * Copy constructor for the entity:
+     * <ul>
+     * <li>copy flag</li>
+     * <li>copy name</li>
+     * <li>deep copy of the setup</li>
+     * <li>copy the working attribute</li>
+     * </ul>
+     *
+     * @param x the entity to be copied
+     */
+    public AuthorizationEntity(AuthorizationEntity x) {
+        flag = x.flag;
+        name = x.name;
+        setup = new TreeMap<>(x.setup);
+        working = x.working;
+    }
+
+    public AuthorizationEntity(AuthControlFlag flag, String name) {
+        this.flag = flag;
+        this.name = name;
+    }
+
     /**
      * Load this entity with given parameters.
      *
@@ -102,6 +128,14 @@ public abstract class AuthorizationEntity implements Nameable, Serializable {
      */
     abstract public boolean setPlugin(IAuthorizationPlugin plugin);
 
+    /**
+     * Perform a deep copy of the entity.
+     *
+     * @return the new instance of this entity
+     */
+    @Override
+    abstract public AuthorizationEntity clone();
+
     /**
      * Get the value of flag
      *

src/org/opensolaris/opengrok/authorization/AuthorizationFramework.java

@@ -203,7 +203,7 @@ protected String getClassName(IAuthorizationPlugin plugin) {
      *
      * @return the stack containing plugins/other stacks
      */
-    protected synchronized AuthorizationStack getStack() {
+    public synchronized AuthorizationStack getStack() {
         return stack;
     }
 
@@ -212,29 +212,31 @@ protected synchronized AuthorizationStack getStack() {
      *
      * @param s new stack to be used
      */
-    protected synchronized void setStack(AuthorizationStack s) {
+    public synchronized void setStack(AuthorizationStack s) {
         this.stack = s;
     }
 
     /**
      * Add an entity into the plugin stack.
      *
+     * @param stack the stack
      * @param entity the authorization entity (stack or plugin)
      */
-    protected synchronized void addPlugin(AuthorizationEntity entity) {
+    protected void addPlugin(AuthorizationStack stack, AuthorizationEntity entity) {
         if (stack != null) {
             stack.add(entity);
         }
     }
 
     /**
      * Add a plugin into the plugin stack. This has the same effect as invoking
-     * addPlugin(IAuthorizationPlugin, REQUIRED).
+     * addPlugin(stack, IAuthorizationPlugin, REQUIRED).
      *
+     * @param stack the stack
      * @param plugin the authorization plugin
      */
-    public synchronized void addPlugin(IAuthorizationPlugin plugin) {
-        addPlugin(plugin, AuthControlFlag.REQUIRED);
+    public void addPlugin(AuthorizationStack stack, IAuthorizationPlugin plugin) {
+        addPlugin(stack, plugin, AuthControlFlag.REQUIRED);
     }
 
     /**
@@ -252,44 +254,49 @@ public synchronized void addPlugin(IAuthorizationPlugin plugin) {
      * <b>The plugin's load method is NOT invoked at this point</b></p>
      *
      * This has the same effect as invoking addPlugin(new
-     * AuthorizationEntity(flag, getClassName(plugin), plugin).
+     * AuthorizationEntity(stack, flag, getClassName(plugin), plugin).
      *
+     * @param stack the stack
      * @param plugin the authorization plugin
      * @param flag the flag for the new plugin
      */
-    public synchronized void addPlugin(IAuthorizationPlugin plugin, AuthControlFlag flag) {
+    public void addPlugin(AuthorizationStack stack, IAuthorizationPlugin plugin, AuthControlFlag flag) {
         if (stack != null) {
             LOGGER.log(Level.INFO, "Plugin class \"{0}\" was not found in configuration."
                     + " Appending the plugin at the end of the list with flag \"{1}\"",
                     new Object[]{getClassName(plugin), flag});
-            addPlugin(new AuthorizationPlugin(flag, getClassName(plugin), plugin));
+            addPlugin(stack, new AuthorizationPlugin(flag, getClassName(plugin), plugin));
         }
     }
 
     /**
-     * Remove and unload all plugins.
+     * Remove and unload all plugins from the stack.
      *
+     * @param stack the stack
      * @see AuthorizationEntity#unload()
      */
-    public synchronized void removeAll() {
-        unloadAllPlugins();
-        stack = RuntimeEnvironment.getInstance().getPluginStack();
+    public void removeAll(AuthorizationStack stack) {
+        unloadAllPlugins(stack);
     }
 
     /**
-     * Load all plugins. If any plugin has not been loaded yet it is marked as
-     * failed.
+     * Load all plugins in the stack. If any plugin has not been loaded yet it
+     * is marked as failed.
+     *
+     * @param stack the stack
      */
-    public synchronized void loadAllPlugins() {
+    public void loadAllPlugins(AuthorizationStack stack) {
         if (stack != null) {
             stack.load(new TreeMap<>());
         }
     }
 
     /**
-     * Unload all plugins
+     * Unload all plugins in the stack
+     *
+     * @param stack the stack
      */
-    public synchronized void unloadAllPlugins() {
+    public void unloadAllPlugins(AuthorizationStack stack) {
         if (stack != null) {
             stack.unload();
         }
@@ -363,17 +370,18 @@ private IAuthorizationPlugin loadClass(String classname) throws ClassNotFoundExc
     /**
      * Traverse list of files which possibly contain a java class and then
      * traverse a list of jar files to load all classes which are contained
-     * within them. Each class is loaded with {@link #handleLoadClass(String)}
-     * which delegates the loading to the custom class loader
-     * {@link #loadClass(String)}.
+     * within them into the given stack. Each class is loaded with
+     * {@link #handleLoadClass(String)} which delegates the loading to the
+     * custom class loader {@link #loadClass(String)}.
      *
+     * @param stack the stack where to add the loaded classes
      * @param classfiles list of files which possibly contain a java class
      * @param jarfiles list of jar files containing java classes
      *
      * @see #handleLoadClass(String)
      * @see #loadClass(String)
      */
-    private void loadClasses(List<File> classfiles, List<File> jarfiles) {
+    private void loadClasses(AuthorizationStack stack, List<File> classfiles, List<File> jarfiles) {
         IAuthorizationPlugin pf;
         for (File file : classfiles) {
             String classname = getClassName(file);
@@ -383,7 +391,7 @@ private void loadClasses(List<File> classfiles, List<File> jarfiles) {
             // load the class in memory and try to find a configured space for this class
             if ((pf = handleLoadClass(classname)) != null && !stack.setPlugin(pf)) {
                 // if there is not configured space -> append it to the stack
-                addPlugin(pf);
+                addPlugin(stack, pf);
             }
         }
 
@@ -399,7 +407,7 @@ private void loadClasses(List<File> classfiles, List<File> jarfiles) {
                     // load the class in memory and try to find a configured space for this class
                     if ((pf = handleLoadClass(classname)) != null && !stack.setPlugin(pf)) {
                         // if there is not configured space -> append it to the stack
-                        addPlugin(pf);
+                        addPlugin(stack, pf);
                     }
                 }
             } catch (IOException ex) {
@@ -433,7 +441,7 @@ private String getClassName(JarEntry f) {
      * @see Configuration#getPluginDirectory()
      */
     @SuppressWarnings("unchecked")
-    public synchronized void reload() {
+    public void reload() {
         if (pluginDirectory == null || !pluginDirectory.isDirectory() || !pluginDirectory.canRead()) {
             LOGGER.log(Level.WARNING, "Plugin directory not found or not readable: {0}. "
                     + "All requests allowed.", pluginDirectory);
@@ -455,22 +463,35 @@ public Object run() {
             }
         });
 
-        // clean the stack
-        removeAll();
+        // clone a new stack not interfering with the current stack
+        AuthorizationStack newStack = RuntimeEnvironment.getInstance().getPluginStack().clone();
 
         // increase the current plugin version tracked by the framework
         increasePluginVersion();
 
-        // refresh the current configuration if there was any change
-        stack = RuntimeEnvironment.getInstance().getPluginStack();
-
         // load all other possible plugin classes
-        loadClasses(
+        loadClasses(newStack,
                 IOUtils.listFilesRec(pluginDirectory, ".class"),
                 IOUtils.listFiles(pluginDirectory, ".jar"));
 
         // fire load events
-        loadAllPlugins();
+        loadAllPlugins(newStack);
+
+        /**
+         * Replace the stack in a synchronized block to avoid inconsistent state
+         * between the stack change and currently executing requests performing
+         * some authorization on the same stack.
+         *
+         * @see #performCheck is also marked as synchronized
+         */
+        AuthorizationStack oldStack = stack;
+        synchronized (this) {
+            stack = newStack;
+        }
+
+        // clean the old stack
+        removeAll(oldStack);
+        oldStack = null;
     }
 
     /**
@@ -578,7 +599,15 @@ private boolean checkAll(HttpServletRequest request, String cache, Nameable enti
         return overallDecision;
     }
 
-    private boolean performCheck(Nameable entity, Predicate<IAuthorizationPlugin> predicate) {
+    /**
+     * Perform the actual check for the entity.
+     *
+     * @param entity either a project or a group
+     * @param predicate a predicate that decides if the authorization is
+     * successful for the given plugin
+     * @return true if entity is allowed; false otherwise
+     */
+    private synchronized boolean performCheck(Nameable entity, Predicate<IAuthorizationPlugin> predicate) {
         return stack.isAllowed(entity, predicate);
     }
 }

src/org/opensolaris/opengrok/authorization/AuthorizationPlugin.java

@@ -49,6 +49,20 @@ public class AuthorizationPlugin extends AuthorizationStack {
     public AuthorizationPlugin() {
     }
 
+    /**
+     * Clone the plugin:
+     * <ul>
+     * <li>copy the superclass {@link AuthorizationStack}</li>
+     * <li>sets the plugin to {@code null}</li>
+     * </ul>
+     *
+     * @param x the plugin to be copied
+     */
+    public AuthorizationPlugin(AuthorizationPlugin x) {
+        super(x);
+        plugin = null;
+    }
+
     public AuthorizationPlugin(AuthControlFlag flag, IAuthorizationPlugin plugin) {
         this(flag, plugin.getClass().getCanonicalName() == null ? plugin.getClass().getName() : plugin.getClass().getCanonicalName(), plugin);
     }
@@ -217,4 +231,18 @@ public boolean isWorking() {
     public boolean hasPlugin() {
         return plugin != null;
     }
+
+    /**
+     * Clone the plugin:
+     * <ul>
+     * <li>copy the superclass {@link AuthorizationStack}</li>
+     * <li>sets the plugin to {@code null}</li>
+     * </ul>
+     *
+     * @return new instance of {@link AuthorizationPlugin}
+     */
+    @Override
+    public AuthorizationPlugin clone() {
+        return new AuthorizationPlugin(this);
+    }
 }

src/org/opensolaris/opengrok/authorization/AuthorizationStack.java

@@ -51,9 +51,26 @@ public class AuthorizationStack extends AuthorizationEntity {
     public AuthorizationStack() {
     }
 
+    /**
+     * Copy constructor from another stack
+     * <ul>
+     * <li>copy the superclass {@link AuthorizationEntity}</li>
+     * <li>perform a deep copy of the contained stack (using
+     * {@link AuthorizationEntity#clone()}</li>
+     * </ul>
+     *
+     * @param x the stack to be copied
+     */
+    public AuthorizationStack(AuthorizationStack x) {
+        super(x);
+        stack = new ArrayList<>(x.stack.size());
+        for (AuthorizationEntity e : x.getStack()) {
+            stack.add(e.clone());
+        }
+    }
+
     public AuthorizationStack(AuthControlFlag flag, String name) {
-        this.flag = flag;
-        this.name = name;
+        super(flag, name);
     }
 
     /**
@@ -241,4 +258,18 @@ public boolean setPlugin(IAuthorizationPlugin plugin) {
         }
         return ret;
     }
+
+    /**
+     * Clone the stack:
+     * <ul>
+     * <li>copy the superclass {@link AuthorizationEntity}</li>
+     * <li>perform a deep copy of the contained stack</li>
+     * </ul>
+     *
+     * @return new instance of {@link AuthorizationStack}
+     */
+    @Override
+    public AuthorizationStack clone() {
+        return new AuthorizationStack(this);
+    }
 }

test/org/opensolaris/opengrok/authorization/AuthorizationFrameworkTest.java

@@ -653,7 +653,7 @@ public static StackSetup[][] params() {
     public void testPluginsGeneric() {
         AuthorizationFramework instance = getInstance();
         instance.setStack(setup.stack);
-        instance.loadAllPlugins();
+        instance.loadAllPlugins(setup.stack);
 
         boolean actual;
         String format = "%s <%s> was <%s> for entity %s";
@@ -685,8 +685,7 @@ public static void tearDownClass() {
     }
 
     static private AuthorizationFramework getInstance() {
-        AuthorizationFramework.getInstance().removeAll();
-        RuntimeEnvironment.getInstance().setPluginStack(new AuthorizationStack(AuthControlFlag.REQUIRED, "default stack"));
+        AuthorizationFramework.getInstance().removeAll(AuthorizationFramework.getInstance().getStack());
         return AuthorizationFramework.getInstance();
     }
 

test/org/opensolaris/opengrok/web/ProjectHelperTestBase.java

@@ -256,12 +256,12 @@ public static void tearDownClass() {
     }
 
     protected void invokeRemoveAll() {
-        env.setPluginStack(new AuthorizationStack(AuthControlFlag.REQUIRED, "default-stack"));
-        AuthorizationFramework.getInstance().removeAll();
+        AuthorizationFramework.getInstance().removeAll(AuthorizationFramework.getInstance().getStack());
+        AuthorizationFramework.getInstance().setStack(new AuthorizationStack(AuthControlFlag.REQUIRED, "default-stack"));
     }
 
     protected void invokeAddPlugin(IAuthorizationPlugin plugin) {
-        AuthorizationFramework.getInstance().addPlugin(plugin);
+        AuthorizationFramework.getInstance().addPlugin(AuthorizationFramework.getInstance().getStack(), plugin);
     }
 
     protected AuthorizationFramework getInstance() {