felix-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From dav...@apache.org
Subject svn commit: r1632257 - in /felix/trunk/http/base/src: main/java/org/apache/felix/http/base/internal/handler/HandlerRegistry.java test/java/org/apache/felix/http/base/internal/handler/HandlerRegistryTest.java
Date Thu, 16 Oct 2014 09:35:25 GMT
Author: davidb
Date: Thu Oct 16 09:35:25 2014
New Revision: 1632257

URL: http://svn.apache.org/r1632257
Log:
[FELIX-4670] Deadlock in Felix HTTP service

This commit removes the locking in the HandlerRegistry and replaces it with the use of ConcurrentMaps.
The HandlerRegistry lock is one of the two locks involved in the deadlock described in FELIX-4670.
Unit tests included.

Added:
    felix/trunk/http/base/src/test/java/org/apache/felix/http/base/internal/handler/HandlerRegistryTest.java
Modified:
    felix/trunk/http/base/src/main/java/org/apache/felix/http/base/internal/handler/HandlerRegistry.java

Modified: felix/trunk/http/base/src/main/java/org/apache/felix/http/base/internal/handler/HandlerRegistry.java
URL: http://svn.apache.org/viewvc/felix/trunk/http/base/src/main/java/org/apache/felix/http/base/internal/handler/HandlerRegistry.java?rev=1632257&r1=1632256&r2=1632257&view=diff
==============================================================================
--- felix/trunk/http/base/src/main/java/org/apache/felix/http/base/internal/handler/HandlerRegistry.java
(original)
+++ felix/trunk/http/base/src/main/java/org/apache/felix/http/base/internal/handler/HandlerRegistry.java
Thu Oct 16 09:35:25 2014
@@ -16,78 +16,86 @@
  */
 package org.apache.felix.http.base.internal.handler;
 
-import org.osgi.service.http.NamespaceException;
-import javax.servlet.ServletException;
-import javax.servlet.Servlet;
-import javax.servlet.Filter;
-import java.util.HashMap;
-import java.util.Map;
 import java.util.Arrays;
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
+import javax.servlet.Filter;
+import javax.servlet.Servlet;
+import javax.servlet.ServletException;
+
+import org.osgi.service.http.NamespaceException;
 
 public final class HandlerRegistry
 {
-    private final Map<Servlet, ServletHandler> servletMap;
-    private final Map<Filter, FilterHandler> filterMap;
-    private final Map<String, Servlet> aliasMap;
-    private ServletHandler[] servlets;
-    private FilterHandler[] filters;
+    private final ConcurrentMap<Servlet, ServletHandler> servletMap = new ConcurrentHashMap<Servlet,
ServletHandler>();
+    private final ConcurrentMap<Filter, FilterHandler> filterMap = new ConcurrentHashMap<Filter,
FilterHandler>();
+    private final ConcurrentMap<String, Servlet> aliasMap = new ConcurrentHashMap<String,
Servlet>();
+    private volatile ServletHandler[] servlets;
+    private volatile FilterHandler[] filters;
 
     public HandlerRegistry()
     {
-        this.servletMap = new HashMap<Servlet, ServletHandler>();
-        this.filterMap = new HashMap<Filter, FilterHandler>();
-        this.aliasMap = new HashMap<String, Servlet>();
-        this.servlets = new ServletHandler[0];
-        this.filters = new FilterHandler[0];
+        servlets = new ServletHandler[0];
+        filters = new FilterHandler[0];
     }
 
     public ServletHandler[] getServlets()
     {
-        return this.servlets;
+        return servlets;
     }
 
     public FilterHandler[] getFilters()
     {
-        return this.filters;
+        return filters;
     }
 
-    public synchronized void addServlet(ServletHandler handler) throws ServletException,
NamespaceException
+    public void addServlet(ServletHandler handler) throws ServletException, NamespaceException
     {
-        if (this.servletMap.containsKey(handler.getServlet()))
+        handler.init();
+
+        // there is a window of opportunity that the servlet/alias was registered between
the
+        // previous check and this one, so we have to atomically add if absent here.
+        if (servletMap.putIfAbsent(handler.getServlet(), handler) != null)
         {
+            // Do not destroy the servlet as the same instance was already registered
             throw new ServletException("Servlet instance already registered");
         }
-
-        if (this.aliasMap.containsKey(handler.getAlias()))
+        if (aliasMap.putIfAbsent(handler.getAlias(), handler.getServlet()) != null)
         {
+            // Remove it from the servletmap too
+            servletMap.remove(handler.getServlet(), handler);
+
+            handler.destroy();
             throw new NamespaceException("Servlet with alias already registered");
         }
 
-        handler.init();
-        this.servletMap.put(handler.getServlet(), handler);
-        this.aliasMap.put(handler.getAlias(), handler.getServlet());
         updateServletArray();
     }
 
-    public synchronized void addFilter(FilterHandler handler) throws ServletException
+    public void addFilter(FilterHandler handler) throws ServletException
     {
-        if (this.filterMap.containsKey(handler.getFilter()))
+        handler.init();
+
+        // there is a window of opportunity that the servlet/alias was registered between
the
+        // previous check and this one, so we have to atomically add if absent here.
+        if (filterMap.putIfAbsent(handler.getFilter(), handler) != null)
         {
+            // Do not destroy the filter as the same instance was already registered
             throw new ServletException("Filter instance already registered");
         }
 
-        handler.init();
-        this.filterMap.put(handler.getFilter(), handler);
         updateFilterArray();
     }
 
-    public synchronized void removeServlet(Servlet servlet, final boolean destroy)
+    public void removeServlet(Servlet servlet, final boolean destroy)
     {
-        ServletHandler handler = this.servletMap.remove(servlet);
+        ServletHandler handler = servletMap.remove(servlet);
         if (handler != null)
         {
             updateServletArray();
-            this.aliasMap.remove(handler.getAlias());
+            aliasMap.remove(handler.getAlias());
             if (destroy)
             {
                 handler.destroy();
@@ -95,9 +103,9 @@ public final class HandlerRegistry
         }
     }
 
-    public synchronized void removeFilter(Filter filter, final boolean destroy)
+    public void removeFilter(Filter filter, final boolean destroy)
     {
-        FilterHandler handler = this.filterMap.remove(filter);
+        FilterHandler handler = filterMap.remove(filter);
         if (handler != null)
         {
             updateFilterArray();
@@ -108,42 +116,44 @@ public final class HandlerRegistry
         }
     }
 
-    public synchronized Servlet getServletByAlias(String alias)
+    public Servlet getServletByAlias(String alias)
     {
-        return this.aliasMap.get(alias);
+        return aliasMap.get(alias);
     }
 
-    public synchronized void removeAll()
+    public void removeAll()
     {
-        for (ServletHandler handler : this.servletMap.values())
+        for (Iterator<ServletHandler> it = servletMap.values().iterator(); it.hasNext();
)
         {
+            ServletHandler handler = it.next();
+            it.remove();
+
+            aliasMap.remove(handler.getAlias());
             handler.destroy();
         }
 
-        for (FilterHandler handler : this.filterMap.values())
+        for (Iterator<FilterHandler> it = filterMap.values().iterator(); it.hasNext();
)
         {
+            FilterHandler handler = it.next();
+            it.remove();
             handler.destroy();
         }
 
-        this.servletMap.clear();
-        this.filterMap.clear();
-        this.aliasMap.clear();
-
         updateServletArray();
         updateFilterArray();
     }
 
     private void updateServletArray()
     {
-        ServletHandler[] tmp = this.servletMap.values().toArray(new ServletHandler[this.servletMap.size()]);
+        ServletHandler[] tmp = servletMap.values().toArray(new ServletHandler[servletMap.size()]);
         Arrays.sort(tmp);
-        this.servlets = tmp;
+        servlets = tmp;
     }
 
     private void updateFilterArray()
     {
-        FilterHandler[] tmp = this.filterMap.values().toArray(new FilterHandler[this.filterMap.size()]);
+        FilterHandler[] tmp = filterMap.values().toArray(new FilterHandler[filterMap.size()]);
         Arrays.sort(tmp);
-        this.filters = tmp;
+        filters = tmp;
     }
 }

Added: felix/trunk/http/base/src/test/java/org/apache/felix/http/base/internal/handler/HandlerRegistryTest.java
URL: http://svn.apache.org/viewvc/felix/trunk/http/base/src/test/java/org/apache/felix/http/base/internal/handler/HandlerRegistryTest.java?rev=1632257&view=auto
==============================================================================
--- felix/trunk/http/base/src/test/java/org/apache/felix/http/base/internal/handler/HandlerRegistryTest.java
(added)
+++ felix/trunk/http/base/src/test/java/org/apache/felix/http/base/internal/handler/HandlerRegistryTest.java
Thu Oct 16 09:35:25 2014
@@ -0,0 +1,262 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.felix.http.base.internal.handler;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.fail;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterConfig;
+import javax.servlet.Servlet;
+import javax.servlet.ServletConfig;
+import javax.servlet.ServletException;
+
+import org.junit.Test;
+import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+import org.osgi.service.http.NamespaceException;
+
+public class HandlerRegistryTest
+{
+    @Test
+    public void testAddRemoveServlet() throws Exception
+    {
+        HandlerRegistry hr = new HandlerRegistry();
+
+        Servlet servlet = Mockito.mock(Servlet.class);
+        ServletHandler handler = new ServletHandler(null, servlet, "/foo", "foo");
+        assertEquals("Precondition", 0, hr.getServlets().length);
+        hr.addServlet(handler);
+        Mockito.verify(servlet, Mockito.times(1)).init(Mockito.any(ServletConfig.class));
+        assertEquals(1, hr.getServlets().length);
+        assertSame(handler, hr.getServlets()[0]);
+
+        ServletHandler handler2 = new ServletHandler(null, servlet, "/bar", "bar");
+        try
+        {
+            hr.addServlet(handler2);
+            fail("Should not have allowed to add the same servlet twice");
+        }
+        catch (ServletException se)
+        {
+            // good
+        }
+        assertArrayEquals(new ServletHandler[] {handler}, hr.getServlets());
+
+        ServletHandler handler3 = new ServletHandler(null, Mockito.mock(Servlet.class),
+                "/foo", "zar");
+        try
+        {
+            hr.addServlet(handler3);
+            fail("Should not have allowed to add the same alias twice");
+        }
+        catch (NamespaceException ne) {
+            // good
+        }
+        assertArrayEquals(new ServletHandler[] {handler}, hr.getServlets());
+
+        assertSame(servlet, hr.getServletByAlias("/foo"));
+
+        Mockito.verify(servlet, Mockito.never()).destroy();
+        hr.removeServlet(servlet, true);
+        Mockito.verify(servlet, Mockito.times(1)).destroy();
+        assertEquals(0, hr.getServlets().length);
+    }
+
+    @Test
+    public void testAddServletWhileSameServletAddedDuringInit() throws Exception
+    {
+        final HandlerRegistry hr = new HandlerRegistry();
+
+        Servlet servlet = Mockito.mock(Servlet.class);
+        final ServletHandler otherHandler = new ServletHandler(null, servlet, "/bar", "bar");
+
+        Mockito.doAnswer(new Answer<Void>()
+        {
+            boolean registered = false;
+            public Void answer(InvocationOnMock invocation) throws Throwable
+            {
+                if (!registered)
+                {
+                    registered = true;
+                    // sneakily register another handler with this servlet before this
+                    // one has finished calling init()
+                    hr.addServlet(otherHandler);
+                }
+                return null;
+            }
+        }).when(servlet).init(Mockito.any(ServletConfig.class));
+
+        ServletHandler handler = new ServletHandler(null, servlet, "/foo", "foo");
+
+        try
+        {
+            hr.addServlet(handler);
+            fail("Should not have allowed the servlet to be added as it was already "
+                    + "added before init was finished");
+
+        }
+        catch (ServletException ne)
+        {
+            // good
+        }
+        assertArrayEquals(new ServletHandler[] {otherHandler}, hr.getServlets());
+    }
+
+    @Test
+    public void testAddServletWhileSameAliasAddedDuringInit() throws Exception
+    {
+        final HandlerRegistry hr = new HandlerRegistry();
+
+        Servlet otherServlet = Mockito.mock(Servlet.class);
+        final ServletHandler otherHandler = new ServletHandler(null, otherServlet, "/foo",
"bar");
+
+        Servlet servlet = Mockito.mock(Servlet.class);
+        Mockito.doAnswer(new Answer<Void>()
+        {
+            public Void answer(InvocationOnMock invocation) throws Throwable
+            {
+                // sneakily register another servlet before this one has finished calling
init()
+                hr.addServlet(otherHandler);
+                return null;
+            }
+        }).when(servlet).init(Mockito.any(ServletConfig.class));
+
+        ServletHandler handler = new ServletHandler(null, servlet, "/foo", "foo");
+
+        try
+        {
+            hr.addServlet(handler);
+            fail("Should not have allowed the servlet to be added as another one got in there
with the same alias");
+        }
+        catch (NamespaceException ne)
+        {
+            // good
+        }
+        assertArrayEquals(new ServletHandler[] {otherHandler}, hr.getServlets());
+        Mockito.verify(servlet, Mockito.times(1)).destroy();
+
+        assertSame(otherServlet, hr.getServletByAlias("/foo"));
+    }
+
+    @Test
+    public void testAddRemoveFilter() throws Exception
+    {
+        HandlerRegistry hr = new HandlerRegistry();
+
+        Filter filter = Mockito.mock(Filter.class);
+        FilterHandler handler = new FilterHandler(null, filter, "/aha", 1, "oho");
+        assertEquals("Precondition", 0, hr.getFilters().length);
+        hr.addFilter(handler);
+        Mockito.verify(filter, Mockito.times(1)).init(Mockito.any(FilterConfig.class));
+        assertEquals(1, hr.getFilters().length);
+        assertSame(handler, hr.getFilters()[0]);
+
+        FilterHandler handler2 = new FilterHandler(null, filter, "/hihi", 2, "haha");
+        try
+        {
+            hr.addFilter(handler2);
+            fail("Should not have allowed the same filter to be added twice");
+        }
+        catch(ServletException se)
+        {
+            // good
+        }
+        assertArrayEquals(new FilterHandler[] {handler}, hr.getFilters());
+
+        Mockito.verify(filter, Mockito.never()).destroy();
+        hr.removeFilter(filter, true);
+        Mockito.verify(filter, Mockito.times(1)).destroy();
+        assertEquals(0, hr.getServlets().length);
+    }
+
+    @Test
+    public void testAddFilterWhileSameFilterAddedDuringInit() throws Exception
+    {
+        final HandlerRegistry hr = new HandlerRegistry();
+
+        Filter filter = Mockito.mock(Filter.class);
+        final FilterHandler otherHandler = new FilterHandler(null, filter, "/two", 99, "two");
+
+        Mockito.doAnswer(new Answer<Void>()
+        {
+            boolean registered = false;
+            public Void answer(InvocationOnMock invocation) throws Throwable
+            {
+                if (!registered)
+                {
+                    registered = true;
+                    // sneakily register another handler with this filter before this
+                    // one has finished calling init()
+                    hr.addFilter(otherHandler);
+                }
+                return null;
+            }
+        }).when(filter).init(Mockito.any(FilterConfig.class));
+
+        FilterHandler handler = new FilterHandler(null, filter, "/one", 1, "one");
+
+        try
+        {
+            hr.addFilter(handler);
+            fail("Should not have allowed the filter to be added as it was already "
+                    + "added before init was finished");
+        }
+        catch (ServletException se)
+        {
+            // good
+        }
+        assertArrayEquals(new FilterHandler[] {otherHandler}, hr.getFilters());
+    }
+
+    @Test
+    public void testRemoveAll() throws Exception
+    {
+        HandlerRegistry hr = new HandlerRegistry();
+
+        Servlet servlet = Mockito.mock(Servlet.class);
+        ServletHandler servletHandler = new ServletHandler(null, servlet, "/f", "f");
+        hr.addServlet(servletHandler);
+        Servlet servlet2 = Mockito.mock(Servlet.class);
+        ServletHandler servletHandler2 = new ServletHandler(null, servlet2, "/ff", "ff");
+        hr.addServlet(servletHandler2);
+        Filter filter = Mockito.mock(Filter.class);
+        FilterHandler filterHandler = new FilterHandler(null, filter, "/f", 0, "f");
+        hr.addFilter(filterHandler);
+
+        assertEquals(2, hr.getServlets().length);
+        assertEquals("Most specific Alias should come first",
+                "/ff", hr.getServlets()[0].getAlias());
+        assertEquals("/f", hr.getServlets()[1].getAlias());
+        assertEquals(1, hr.getFilters().length);
+        assertSame(filter, hr.getFilters()[0].getFilter());
+
+        Mockito.verify(servlet, Mockito.never()).destroy();
+        Mockito.verify(servlet2, Mockito.never()).destroy();
+        Mockito.verify(filter, Mockito.never()).destroy();
+        hr.removeAll();
+        Mockito.verify(servlet, Mockito.times(1)).destroy();
+        Mockito.verify(servlet2, Mockito.times(1)).destroy();
+        Mockito.verify(filter, Mockito.times(1)).destroy();
+
+        assertEquals(0, hr.getServlets().length);
+        assertEquals(0, hr.getFilters().length);
+    }
+}



Mime
View raw message