Author: dhanji
Date: Sat Feb 14 02:28:08 2009
New Revision: 846
Modified:
trunk/servlet/src/com/google/inject/servlet/FilterDefinition.java
trunk/servlet/src/com/google/inject/servlet/ManagedFilterPipeline.java
trunk/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java
trunk/servlet/src/com/google/inject/servlet/ServletDefinition.java
trunk/servlet/test/com/google/inject/servlet/FilterDefinitionTest.java
trunk/servlet/test/com/google/inject/servlet/ServletDefinitionPathsTest.java
trunk/servlet/test/com/google/inject/servlet/ServletDefinitionTest.java
trunk/servlet/test/com/google/inject/servlet/ServletDispatchIntegrationTest.java
trunk/servlet/test/com/google/inject/servlet/ServletPipelineRequestDispatcherTest.java
trunk/servlet/test/com/google/inject/servlet/VarargsServletDispatchIntegrationTest.java
Log:
Critical bug fix for issue:
http://code.google.com/p/google-guice/issues/detail?id=335
Modified: trunk/servlet/src/com/google/inject/servlet/FilterDefinition.java
==============================================================================
--- trunk/servlet/src/com/google/inject/servlet/FilterDefinition.java
(original)
+++ trunk/servlet/src/com/google/inject/servlet/FilterDefinition.java Sat
Feb 14 02:28:08 2009
@@ -24,6 +24,7 @@
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Map;
+import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
import javax.servlet.Filter;
import javax.servlet.FilterConfig;
@@ -58,7 +59,9 @@
return patternMatcher.matches(uri);
}
- public void init(final ServletContext servletContext, Injector injector)
throws ServletException {
+ public void init(final ServletContext servletContext, Injector injector,
+ Set<Filter> initializedSoFar) throws ServletException {
+
// This absolutely must be a singleton, and so is only initialized
once.
if (!isSingletonBinding(injector.getBinding(filterKey))) {
throw new ServletException("Filters must be bound as singletons. "
@@ -68,6 +71,12 @@
Filter filter = injector.getInstance(filterKey);
this.filter.set(filter);
+ // Only fire init() if this Singleton filter has not already appeared
earlier
+ // in the filter chain.
+ if (initializedSoFar.contains(filter)) {
+ return;
+ }
+
//initialize our filter with the configured context params and servlet
context
filter.init(new FilterConfig() {
public String getFilterName() {
@@ -86,19 +95,28 @@
return Iterators.asEnumeration(initParams.keySet().iterator());
}
});
+
+ initializedSoFar.add(filter);
}
- public void destroy() {
+ public void destroy(Set<Filter> destroyedSoFar) {
// filters are always singletons
Filter reference = filter.get();
// Do nothing if this Filter was invalid (usually due to not being
scoped
- // properly). According to Servlet Spec: it is "out of service", and
does not
- // need to be destroyed.
- if (null == reference) {
+ // properly), or was already destroyed. According to Servlet Spec: it
is
+ // "out of service", and does not need to be destroyed.
+ // Also prevent duplicate destroys to the same singleton that may
appear
+ // more than once on the filter chain.
+ if (null == reference || destroyedSoFar.contains(reference)) {
return;
}
- reference.destroy();
+
+ try {
+ reference.destroy();
+ } finally {
+ destroyedSoFar.add(reference);
+ }
}
public void doFilter(ServletRequest servletRequest,
Modified:
trunk/servlet/src/com/google/inject/servlet/ManagedFilterPipeline.java
==============================================================================
--- trunk/servlet/src/com/google/inject/servlet/ManagedFilterPipeline.java
(original)
+++ trunk/servlet/src/com/google/inject/servlet/ManagedFilterPipeline.java
Sat Feb 14 02:28:08 2009
@@ -15,7 +15,9 @@
*/
package com.google.inject.servlet;
+import com.google.common.base.ReferenceType;
import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
import com.google.inject.Binding;
import com.google.inject.Inject;
import com.google.inject.Injector;
@@ -26,6 +28,8 @@
import java.io.IOException;
import java.util.Collections;
import java.util.List;
+import java.util.Set;
+import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.RequestDispatcher;
import javax.servlet.ServletContext;
@@ -91,8 +95,11 @@
if (initialized)
return;
+ // Used to prevent duplicate initialization.
+ Set<Filter> initializedSoFar =
Sets.newIdentityHashSet(ReferenceType.STRONG);
+
for (FilterDefinition filterDefinition : filterDefinitions) {
- filterDefinition.init(servletContext, injector);
+ filterDefinition.init(servletContext, injector, initializedSoFar);
}
//next, initialize servlets...
@@ -151,9 +158,9 @@
servletPipeline.destroy();
//go down chain and destroy all our filters
- //TODO check servlet spec if we should continue destroying even if
exceptions are thrown
+ Set<Filter> destroyedSoFar =
Sets.newIdentityHashSet(ReferenceType.STRONG);
for (FilterDefinition filterDefinition : filterDefinitions) {
- filterDefinition.destroy();
+ filterDefinition.destroy(destroyedSoFar);
}
}
}
Modified:
trunk/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java
==============================================================================
--- trunk/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java
(original)
+++ trunk/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java
Sat Feb 14 02:28:08 2009
@@ -15,7 +15,10 @@
*/
package com.google.inject.servlet;
+import com.google.common.base.Preconditions;
+import com.google.common.base.ReferenceType;
import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
import com.google.inject.Binding;
import com.google.inject.Inject;
import com.google.inject.Injector;
@@ -25,11 +28,13 @@
import java.io.IOException;
import java.util.Collections;
import java.util.List;
+import java.util.Set;
import javax.servlet.RequestDispatcher;
import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServlet;
/**
* A wrapping dispatcher for servlets, in much the same way as {...@link
ManagedFilterPipeline} is for
@@ -68,8 +73,10 @@
}
public void init(ServletContext servletContext, Injector injector)
throws ServletException {
+ Set<HttpServlet> initializedSoFar =
Sets.newIdentityHashSet(ReferenceType.STRONG);
+
for (ServletDefinition servletDefinition : servletDefinitions) {
- servletDefinition.init(servletContext, injector);
+ servletDefinition.init(servletContext, injector, initializedSoFar);
}
}
@@ -88,8 +95,9 @@
}
public void destroy() {
+ Set<HttpServlet> destroyedSoFar =
Sets.newIdentityHashSet(ReferenceType.STRONG);
for (ServletDefinition servletDefinition : servletDefinitions) {
- servletDefinition.destroy();
+ servletDefinition.destroy(destroyedSoFar);
}
}
@@ -105,11 +113,9 @@
public void forward(ServletRequest servletRequest,
ServletResponse servletResponse)
throws ServletException, IOException {
- if (servletResponse.isCommitted()) {
- throw new IllegalStateException("Response has been
committed--you can "
- + "only call forward before committing the response
(hint: don't "
- + "flush buffers)");
- }
+ Preconditions.checkState(!servletResponse.isCommitted(),
+ "Response has been committed--you can only call forward
before"
+ + " committing the response (hint: don't flush buffers)");
// clear buffer before forwarding
servletResponse.resetBuffer();
Modified: trunk/servlet/src/com/google/inject/servlet/ServletDefinition.java
==============================================================================
--- trunk/servlet/src/com/google/inject/servlet/ServletDefinition.java
(original)
+++ trunk/servlet/src/com/google/inject/servlet/ServletDefinition.java Sat
Feb 14 02:28:08 2009
@@ -24,6 +24,7 @@
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Map;
+import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
import javax.servlet.ServletConfig;
import javax.servlet.ServletContext;
@@ -61,7 +62,9 @@
return patternMatcher.matches(uri);
}
- public void init(final ServletContext servletContext, Injector injector)
throws ServletException {
+ public void init(final ServletContext servletContext, Injector injector,
+ Set<HttpServlet> initializedSoFar) throws ServletException {
+
// This absolutely must be a singleton, and so is only initialized
once.
if (!isSingletonBinding(injector.getBinding(servletKey))) {
throw new ServletException("Servlets must be bound as singletons. "
@@ -71,6 +74,11 @@
HttpServlet httpServlet = injector.getInstance(servletKey);
this.httpServlet.set(httpServlet);
+ // Only fire init() if we have not appeared before in the filter chain.
+ if (initializedSoFar.contains(httpServlet)) {
+ return;
+ }
+
//initialize our servlet with the configured context params and
servlet context
httpServlet.init(new ServletConfig() {
public String getServletName() {
@@ -89,18 +97,28 @@
return Iterators.asEnumeration(initParams.keySet().iterator());
}
});
+
+ // Mark as initialized.
+ initializedSoFar.add(httpServlet);
}
- public void destroy() {
+ public void destroy(Set<HttpServlet> destroyedSoFar) {
HttpServlet reference = httpServlet.get();
// Do nothing if this Servlet was invalid (usually due to not being
scoped
// properly). According to Servlet Spec: it is "out of service", and
does not
// need to be destroyed.
- if (null == reference) {
+ // Also prevent duplicate destroys to the same singleton that may
appear
+ // more than once on the filter chain.
+ if (null == reference || destroyedSoFar.contains(reference)) {
return;
}
- reference.destroy();
+
+ try {
+ reference.destroy();
+ } finally {
+ destroyedSoFar.add(reference);
+ }
}
/**
Modified:
trunk/servlet/test/com/google/inject/servlet/FilterDefinitionTest.java
==============================================================================
--- trunk/servlet/test/com/google/inject/servlet/FilterDefinitionTest.java
(original)
+++ trunk/servlet/test/com/google/inject/servlet/FilterDefinitionTest.java
Sat Feb 14 02:28:08 2009
@@ -1,5 +1,7 @@
package com.google.inject.servlet;
+import com.google.common.base.ReferenceType;
+import com.google.common.collect.Sets;
import com.google.inject.Binding;
import com.google.inject.Injector;
import com.google.inject.Key;
@@ -64,7 +66,7 @@
replay(servletContext);
- filterDef.init(servletContext, injector);
+ filterDef.init(servletContext, injector,
Sets.<Filter>newIdentityHashSet(ReferenceType.STRONG));
final FilterConfig filterConfig = mockFilter.getConfig();
assert null != filterConfig;
@@ -103,7 +105,8 @@
assert filterDef.getFilter() instanceof MockFilter;
//should fire on mockfilter now
- filterDef.init(createMock(ServletContext.class), injector);
+ filterDef.init(createMock(ServletContext.class), injector,
+ Sets.<Filter>newIdentityHashSet(ReferenceType.STRONG));
assert mockFilter.isInit() : "Init did not fire";
@@ -118,7 +121,7 @@
assertTrue("Filter did not proceed down chain", proceed[0]);
- filterDef.destroy();
+
filterDef.destroy(Sets.<Filter>newIdentityHashSet(ReferenceType.STRONG));
assertTrue("Destroy did not fire", mockFilter.isDestroy());
verify(injector, request);
@@ -157,7 +160,8 @@
assert filterDef.getFilter() instanceof MockFilter;
//should fire on mockfilter now
- filterDef.init(createMock(ServletContext.class), injector);
+ filterDef.init(createMock(ServletContext.class), injector,
+ Sets.<Filter>newIdentityHashSet(ReferenceType.STRONG));
assert mockFilter.isInit() : "Init did not fire";
@@ -171,7 +175,7 @@
assert !proceed[0] : "Filter did not suppress chain";
- filterDef.destroy();
+
filterDef.destroy(Sets.<Filter>newIdentityHashSet(ReferenceType.STRONG));
assert mockFilter.isDestroy() : "Destroy did not fire";
verify(injector, request);
Modified:
trunk/servlet/test/com/google/inject/servlet/ServletDefinitionPathsTest.java
==============================================================================
---
trunk/servlet/test/com/google/inject/servlet/ServletDefinitionPathsTest.java
(original)
+++
trunk/servlet/test/com/google/inject/servlet/ServletDefinitionPathsTest.java
Sat Feb 14 02:28:08 2009
@@ -16,6 +16,8 @@
package com.google.inject.servlet;
+import com.google.common.base.ReferenceType;
+import com.google.common.collect.Sets;
import com.google.inject.Binding;
import com.google.inject.Injector;
import com.google.inject.Key;
@@ -86,7 +88,7 @@
ServletDefinition servletDefinition = new ServletDefinition(mapping,
Key.get(HttpServlet.class),
UriPatternType.get(UriPatternType.SERVLET, mapping), new
HashMap<String, String>());
- servletDefinition.init(null, injector);
+ servletDefinition.init(null, injector,
Sets.<HttpServlet>newIdentityHashSet(ReferenceType.STRONG));
servletDefinition.doService(request, response);
assertTrue("Servlet did not run!", run[0]);
@@ -171,7 +173,7 @@
ServletDefinition servletDefinition = new ServletDefinition(mapping,
Key.get(HttpServlet.class),
UriPatternType.get(UriPatternType.SERVLET, mapping), new
HashMap<String, String>());
- servletDefinition.init(null, injector);
+ servletDefinition.init(null, injector,
Sets.<HttpServlet>newIdentityHashSet(ReferenceType.STRONG));
servletDefinition.doService(request, response);
assertTrue("Servlet did not run!", run[0]);
@@ -256,7 +258,7 @@
ServletDefinition servletDefinition = new ServletDefinition(mapping,
Key.get(HttpServlet.class),
UriPatternType.get(UriPatternType.REGEX, mapping), new
HashMap<String, String>());
- servletDefinition.init(null, injector);
+ servletDefinition.init(null, injector,
Sets.<HttpServlet>newIdentityHashSet(ReferenceType.STRONG));
servletDefinition.doService(request, response);
assertTrue("Servlet did not run!", run[0]);
Modified:
trunk/servlet/test/com/google/inject/servlet/ServletDefinitionTest.java
==============================================================================
--- trunk/servlet/test/com/google/inject/servlet/ServletDefinitionTest.java
(original)
+++ trunk/servlet/test/com/google/inject/servlet/ServletDefinitionTest.java
Sat Feb 14 02:28:08 2009
@@ -16,15 +16,11 @@
package com.google.inject.servlet;
-import static org.easymock.EasyMock.createMock;
-import static org.easymock.EasyMock.expect;
-import static org.easymock.EasyMock.replay;
-
+import com.google.common.base.ReferenceType;
+import com.google.common.collect.Sets;
+import com.google.inject.Binding;
import com.google.inject.Injector;
import com.google.inject.Key;
-import com.google.inject.Binding;
-import junit.framework.TestCase;
-
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Map;
@@ -32,6 +28,10 @@
import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
+import junit.framework.TestCase;
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
/**
* Basic unit test for lifecycle of a ServletDefinition (wrapper).
@@ -76,7 +76,7 @@
replay(servletContext);
- servletDefinition.init(servletContext, injector);
+ servletDefinition.init(servletContext, injector,
Sets.<HttpServlet>newIdentityHashSet(ReferenceType.STRONG));
assertNotNull(mockServlet.getServletContext());
assertEquals(contextName,
mockServlet.getServletContext().getServletContextName());
Modified:
trunk/servlet/test/com/google/inject/servlet/ServletDispatchIntegrationTest.java
==============================================================================
---
trunk/servlet/test/com/google/inject/servlet/ServletDispatchIntegrationTest.java
(original)
+++
trunk/servlet/test/com/google/inject/servlet/ServletDispatchIntegrationTest.java
Sat Feb 14 02:28:08 2009
@@ -93,7 +93,7 @@
assertTrue("lifecycle states did not fire correct number of times--
inits: " + inits + "; dos: "
+ services + "; destroys: " + destroys,
- inits == 5 && services == 1 && destroys == 5);
+ inits == 2 && services == 1 && destroys == 2);
}
public final void testDispatchRequestToManagedPipelineWithFilter()
@@ -136,8 +136,8 @@
verify(requestMock);
assertTrue("lifecycle states did not fire correct number of times--
inits: " + inits + "; dos: "
- + services + "; destroys: " + destroys,
- inits == 6 && services == 1 && destroys == 6 && doFilters == 1);
+ + services + "; destroys: " + destroys + "; doFilters: " +
doFilters,
+ inits == 3 && services == 1 && destroys == 3 && doFilters == 1);
}
@Singleton
Modified:
trunk/servlet/test/com/google/inject/servlet/ServletPipelineRequestDispatcherTest.java
==============================================================================
---
trunk/servlet/test/com/google/inject/servlet/ServletPipelineRequestDispatcherTest.java
(original)
+++
trunk/servlet/test/com/google/inject/servlet/ServletPipelineRequestDispatcherTest.java
Sat Feb 14 02:28:08 2009
@@ -16,7 +16,9 @@
package com.google.inject.servlet;
+import com.google.common.base.ReferenceType;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Sets;
import com.google.inject.Binding;
import com.google.inject.Injector;
import com.google.inject.Key;
@@ -94,7 +96,7 @@
replay(injector, mockRequest, mockBinding);
// Have to init the Servlet before we can dispatch to it.
- servletDefinition.init(null, injector);
+ servletDefinition.init(null, injector,
Sets.<HttpServlet>newIdentityHashSet(ReferenceType.STRONG));
final RequestDispatcher dispatcher = new ManagedServletPipeline(
injector)
@@ -158,7 +160,7 @@
replay(injector, mockRequest, mockResponse, mockBinding);
// Have to init the Servlet before we can dispatch to it.
- servletDefinition.init(null, injector);
+ servletDefinition.init(null, injector,
Sets.<HttpServlet>newIdentityHashSet(ReferenceType.STRONG));
final RequestDispatcher dispatcher = new
ManagedServletPipeline(injector)
.getRequestDispatcher(pattern);
@@ -228,7 +230,7 @@
replay(injector, mockRequest, mockResponse, mockBinding);
// Have to init the Servlet before we can dispatch to it.
- servletDefinition.init(null, injector);
+ servletDefinition.init(null, injector,
Sets.<HttpServlet>newIdentityHashSet(ReferenceType.STRONG));
final RequestDispatcher dispatcher = new
ManagedServletPipeline(injector)
.getRequestDispatcher(pattern);
Modified:
trunk/servlet/test/com/google/inject/servlet/VarargsServletDispatchIntegrationTest.java
==============================================================================
---
trunk/servlet/test/com/google/inject/servlet/VarargsServletDispatchIntegrationTest.java
(original)
+++
trunk/servlet/test/com/google/inject/servlet/VarargsServletDispatchIntegrationTest.java
Sat Feb 14 02:28:08 2009
@@ -89,7 +89,7 @@
assertTrue("lifecycle states did not fire correct number of times--
inits: " + inits + "; dos: "
+ services + "; destroys: " + destroys,
- inits == 6 && services == 1 && destroys == 6);
+ inits == 2 && services == 1 && destroys == 2);
}
public final void
testVarargsSkipDispatchRequestToManagedPipelineServlets()
@@ -126,7 +126,7 @@
assertTrue("lifecycle states did not fire correct number of times--
inits: " + inits + "; dos: "
+ services + "; destroys: " + destroys,
- inits == 7 && services == 1 && destroys == 7);
+ inits == 2 && services == 1 && destroys == 2);
}
public final void testDispatchRequestToManagedPipelineWithFilter()
@@ -167,7 +167,7 @@
assertTrue("lifecycle states did not fire correct number of times--
inits: " + inits + "; dos: "
+ services + "; destroys: " + destroys,
- inits == 6 && services == 1 && destroys == 6 && doFilters == 1);
+ inits == 3 && services == 1 && destroys == 3 && doFilters == 1);
}
@Singleton
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"google-guice-dev" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/google-guice-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
