You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@felix.apache.org by da...@apache.org on 2014/10/16 11:35:25 UTC
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
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);
+ }
+}