You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tapestry.apache.org by hl...@apache.org on 2006/07/24 04:01:58 UTC
svn commit: r424878 - in /tapestry/tapestry5/tapestry-core/trunk/src:
main/java/org/apache/tapestry/internal/ioc/
main/resources/org/apache/tapestry/internal/ioc/
test/java/org/apache/tapestry/internal/ioc/
Author: hlship
Date: Sun Jul 23 19:01:58 2006
New Revision: 424878
URL: http://svn.apache.org/viewvc?rev=424878&view=rev
Log:
Add checks to prevent services from depending on themselves (during construction) as this will cause a Java stack overflow.
Added:
tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/OneShotServiceCreator.java
tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/OneShortServiceCreatorTest.java
Modified:
tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/IOCMessages.java
tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/ModuleImpl.java
tapestry/tapestry5/tapestry-core/trunk/src/main/resources/org/apache/tapestry/internal/ioc/IOCStrings.properties
tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTest.java
tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTestModule.java
Modified: tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/IOCMessages.java
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/IOCMessages.java?rev=424878&r1=424877&r2=424878&view=diff
==============================================================================
--- tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/IOCMessages.java (original)
+++ tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/IOCMessages.java Sun Jul 23 19:01:58 2006
@@ -22,6 +22,7 @@
import org.apache.hivemind.Messages;
import org.apache.hivemind.impl.MessageFormatter;
import org.apache.tapestry.internal.annotations.Utility;
+import org.apache.tapestry.ioc.def.ServiceDef;
/**
* @author Howard M. Lewis Ship
@@ -150,13 +151,18 @@
{ asString(method), serviceId, returned, serviceInterface.getName() });
}
- static Object creatingService(String serviceId)
+ static String creatingService(String serviceId)
{
return MESSAGES.format("creating-service", serviceId);
}
- static Object invokingMethod(Method method)
+ static String invokingMethod(Method method)
{
return MESSAGES.format("invoking-method", asString(method));
+ }
+
+ static String recursiveServiceBuild(ServiceDef def)
+ {
+ return MESSAGES.format("recursive-service-build", def.getServiceId(), def.toString());
}
}
Modified: tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/ModuleImpl.java
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/ModuleImpl.java?rev=424878&r1=424877&r2=424878&view=diff
==============================================================================
--- tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/ModuleImpl.java (original)
+++ tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/ModuleImpl.java Sun Jul 23 19:01:58 2006
@@ -53,6 +53,12 @@
private Object _moduleBuilder;
+ /**
+ * Service ids of services that are currently under construction; used to identify when a
+ * primtive service is dependent upon itself (and prevent a stack overflow!).
+ */
+ private final Set<String> _underConstruction = newSet();
+
/** Identifies the internal module. Services within this module are never decorated. */
private static final String INTERNAL_MODULE_ID = "tapestry.ioc";
@@ -180,6 +186,11 @@
String serviceId = def.getServiceId();
+ if (_underConstruction.contains(serviceId))
+ throw new IllegalStateException(IOCMessages.recursiveServiceBuild(def));
+
+ _underConstruction.add(serviceId);
+
Log log = _registry.getLog(serviceId);
if (log.isDebugEnabled())
@@ -205,9 +216,14 @@
if (!_moduleDef.getModuleId().equals(INTERNAL_MODULE_ID))
creator = new InterceptorStackBuilder(this, serviceId, creator);
- // TODO: Add a creator that checks to prevent recursive construction of the service.
- // What would be a scenario where this could occur? I guess builder method for A
- // injects B and invokes a method on B and B itself is dependant on A.
+ // Add a wrapper that makes sure that it only gets created once. For primitive (or
+ // other non-proxy lifecycle)
+ // services, we rely upon _underConstruction to track the ids of services
+ // being created (that's because a recursive call in a primitive service's
+ // service builder method will cause a new ServiceCreator stack to be created,
+ // with an eventual Java stack overflow).
+
+ creator = new OneShotServiceCreator(def, creator);
// For primitive services, we just create the service right here. For non-primitive services
// we create the proxy the defers creation until needed.
@@ -216,6 +232,8 @@
.createService();
_services.put(serviceId, service);
+
+ _underConstruction.remove(serviceId);
return service;
}
Added: tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/OneShotServiceCreator.java
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/OneShotServiceCreator.java?rev=424878&view=auto
==============================================================================
--- tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/OneShotServiceCreator.java (added)
+++ tapestry/tapestry5/tapestry-core/trunk/src/main/java/org/apache/tapestry/internal/ioc/OneShotServiceCreator.java Sun Jul 23 19:01:58 2006
@@ -0,0 +1,45 @@
+package org.apache.tapestry.internal.ioc;
+
+import org.apache.tapestry.ioc.ServiceCreator;
+import org.apache.tapestry.ioc.def.ServiceDef;
+
+/**
+ * Decorator for {@link org.apache.tapestry.ioc.ServiceCreator} that ensures the service is only
+ * created once. This detects a situation where the service builder for a service directly or
+ * indirectly invokes methods on the service itself. This would show up as a second call up the
+ * ServiceCreator stack injected into the proxy.
+ * <p>
+ * We could use the {@link org.apache.tapestry.internal.annotations.OneShot} annotation, but this
+ * implementation gives us a bit more flexibility to report the error.
+ *
+ * @author Howard M. Lewis Ship
+ */
+public class OneShotServiceCreator implements ServiceCreator
+{
+ private final ServiceDef _serviceDef;
+
+ private final ServiceCreator _delegate;
+
+ private boolean _locked;
+
+ public OneShotServiceCreator(ServiceDef serviceDef, ServiceCreator delegate)
+ {
+ _serviceDef = serviceDef;
+ _delegate = delegate;
+ }
+
+ /**
+ * We could make this method synchronized, but in the context of creating a service for a proxy,
+ * it will already be synchronized (inside the proxy).
+ */
+ public Object createService()
+ {
+ if (_locked)
+ throw new IllegalStateException(IOCMessages.recursiveServiceBuild(_serviceDef));
+
+ _locked = true;
+
+ return _delegate.createService();
+ }
+
+}
Modified: tapestry/tapestry5/tapestry-core/trunk/src/main/resources/org/apache/tapestry/internal/ioc/IOCStrings.properties
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/tapestry-core/trunk/src/main/resources/org/apache/tapestry/internal/ioc/IOCStrings.properties?rev=424878&r1=424877&r2=424878&view=diff
==============================================================================
--- tapestry/tapestry5/tapestry-core/trunk/src/main/resources/org/apache/tapestry/internal/ioc/IOCStrings.properties (original)
+++ tapestry/tapestry5/tapestry-core/trunk/src/main/resources/org/apache/tapestry/internal/ioc/IOCStrings.properties Sun Jul 23 19:01:58 2006
@@ -40,4 +40,7 @@
decorator-returned-wrong-type=Decorator method {0} (invoked for service ''{1}'') returned {2}, \
which is not assignable to the {3} service interface.
creating-service=Creating service ''{0}''.
-invoking-method=Invoking method {0}.
\ No newline at end of file
+invoking-method=Invoking method {0}.
+recursive-service-build=Construction of service ''{0}'' has failed due to recursion: \
+ the service depends on itself in some way. \
+ Please check {1} for references to another service that is itself dependent on service ''{0}''.
\ No newline at end of file
Modified: tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTest.java
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTest.java?rev=424878&r1=424877&r2=424878&view=diff
==============================================================================
--- tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTest.java (original)
+++ tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTest.java Sun Jul 23 19:01:58 2006
@@ -14,12 +14,8 @@
package org.apache.tapestry.internal.ioc;
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertTrue;
-
import java.util.Arrays;
import java.util.Collection;
-import java.util.List;
import java.util.Set;
import org.apache.commons.logging.Log;
@@ -33,6 +29,9 @@
import org.apache.tapestry.ioc.services.ClassFactory;
import org.testng.annotations.Test;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
/**
* @author Howard M. Lewis Ship
*/
@@ -166,6 +165,7 @@
verify();
}
+ @SuppressWarnings("unchecked")
@Test
public void find_decorator_defs_for_service()
{
@@ -239,5 +239,44 @@
ToStringService ts = registry.getService(ToStringService.class);
assertEquals(ts.toString(), "<ToStringService: ioc.test.ToString>");
+ }
+
+ @Test
+ public void recursive_singleton_integration_test()
+ {
+ Registry registry = buildRegistry();
+
+ FoeService foe = registry.getService("ioc.test.RecursiveFoe", FoeService.class);
+
+ try
+ {
+ foe.foe();
+ unreachable();
+ }
+ catch (RuntimeException ex)
+ {
+ // The details are checked elsewhere.
+ }
+ }
+
+ @Test
+ public void recursive_primitive_integration_test()
+ {
+ Registry registry = buildRegistry();
+
+ try
+ {
+ registry.getService("ioc.test.PrimitiveRecursiveFoe", FoeService.class);
+ unreachable();
+ }
+ catch (IllegalStateException ex)
+ {
+ assertEquals(
+ ex.getMessage(),
+ "Construction of service 'ioc.test.PrimitiveRecursiveFoe' has failed due to recursion: "
+ + "the service depends on itself in some way. Please check "
+ + ModuleImplTestModule.class.getName()
+ + ".buildPrimitiveRecursiveFoe(FoeService) for references to another service that is itself dependent on service 'ioc.test.PrimitiveRecursiveFoe'.");
+ }
}
}
Modified: tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTestModule.java
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTestModule.java?rev=424878&r1=424877&r2=424878&view=diff
==============================================================================
--- tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTestModule.java (original)
+++ tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/ModuleImplTestModule.java Sun Jul 23 19:01:58 2006
@@ -15,6 +15,8 @@
package org.apache.tapestry.internal.ioc;
import org.apache.tapestry.ioc.annotations.Id;
+import org.apache.tapestry.ioc.annotations.InjectService;
+import org.apache.tapestry.ioc.annotations.Lifecycle;
import org.apache.tapestry.ioc.annotations.Private;
/**
@@ -50,6 +52,27 @@
public FieService buildFie()
{
+ return null;
+ }
+
+ public FoeService buildRecursiveFoe(@InjectService("ioc.test.RecursiveFoe")
+ FoeService self)
+ {
+ // While constructing self, we invoke a method on self.
+
+ self.foe();
+
+ return null;
+ }
+
+ @Lifecycle("primitive")
+ public FoeService buildPrimitiveRecursiveFoe(@InjectService("ioc.test.PrimitiveRecursiveFoe")
+ FoeService self)
+ {
+ // While constructing self, we invoke a method on self.
+
+ self.foe();
+
return null;
}
Added: tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/OneShortServiceCreatorTest.java
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/OneShortServiceCreatorTest.java?rev=424878&view=auto
==============================================================================
--- tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/OneShortServiceCreatorTest.java (added)
+++ tapestry/tapestry5/tapestry-core/trunk/src/test/java/org/apache/tapestry/internal/ioc/OneShortServiceCreatorTest.java Sun Jul 23 19:01:58 2006
@@ -0,0 +1,57 @@
+package org.apache.tapestry.internal.ioc;
+
+import java.lang.reflect.Method;
+
+import org.apache.tapestry.internal.test.InternalBaseTestCase;
+import org.apache.tapestry.ioc.ServiceCreator;
+import org.apache.tapestry.ioc.def.ServiceDef;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertSame;
+
+/**
+ * @author Howard M. Lewis Ship
+ */
+public class OneShortServiceCreatorTest extends InternalBaseTestCase
+{
+ @Test
+ public void ensure_only_called_once() throws Exception
+ {
+ Method method = getClass().getMethod("buildMyService");
+
+ ServiceCreator delegate = newServiceCreator();
+ Object service = new Object();
+
+ ServiceDef def = new ServiceDefImpl("foo.Bar", "singleton", method, false);
+
+ trainCreateService(delegate, service);
+
+ replay();
+
+ ServiceCreator oneShot = new OneShotServiceCreator(def, delegate);
+
+ assertSame(oneShot.createService(), service);
+
+ try
+ {
+ oneShot.createService();
+ unreachable();
+ }
+ catch (IllegalStateException ex)
+ {
+ assertEquals(
+ ex.getMessage(),
+ "Construction of service 'foo.Bar' has failed due to recursion: the service depends on itself in some way. Please check "
+ + getClass().getName()
+ + ".buildMyService() for references to another service that is itself dependent on service 'foo.Bar'.");
+ }
+
+ }
+
+ /** Fake service builder method. */
+ public Runnable buildMyService()
+ {
+ return null;
+ }
+}