Seam SVN: r14976 - in branches/enterprise/JBPAPP_5_0/src: test/integration/src/org/jboss/seam/test/integration and 1 other directory.
by seam-commits＠lists.jboss.org
Author: manaRH
Date: 2012-07-03 06:34:31 -0400 (Tue, 03 Jul 2012)
New Revision: 14976
Added:
branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/FactoryLockTest.java
Modified:
branches/enterprise/JBPAPP_5_0/src/main/org/jboss/seam/Component.java
branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/ConcurrentFactoryTest.java
branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/testng.xml
Log:
JBPAPP-9391 using factory lock much less
Modified: branches/enterprise/JBPAPP_5_0/src/main/org/jboss/seam/Component.java
===================================================================
--- branches/enterprise/JBPAPP_5_0/src/main/org/jboss/seam/Component.java 2012-07-03 10:33:05 UTC (rev 14975)
+++ branches/enterprise/JBPAPP_5_0/src/main/org/jboss/seam/Component.java 2012-07-03 10:34:31 UTC (rev 14976)
@@ -50,7 +50,6 @@
import java.util.SortedSet;
import java.util.TreeMap;
import java.util.TreeSet;
-import java.util.concurrent.locks.ReentrantLock;
import javassist.util.proxy.MethodFilter;
import javassist.util.proxy.MethodHandler;
@@ -130,8 +129,6 @@
private static final LogProvider log = Logging.getLogProvider(Component.class);
- static ReentrantLock factoryLock = new ReentrantLock();
-
private ComponentType type;
private String name;
private ScopeType scope;
@@ -2057,54 +2054,72 @@
private static Object getInstanceFromFactory(String name, ScopeType scope)
{
Init init = Init.instance();
- if (init==null) //for unit tests, yew!
+ if (init == null) // for unit tests, yew!
{
return null;
}
else
{
- Init.FactoryMethod factoryMethod = init.getFactory(name);
+ Init.FactoryMethod factoryMethod = init.getFactory(name);
Init.FactoryExpression methodBinding = init.getFactoryMethodExpression(name);
Init.FactoryExpression valueBinding = init.getFactoryValueExpression(name);
- if ( methodBinding!=null && getOutScope( methodBinding.getScope(), null ).isContextActive() ) //let the XML take precedence
- {
+ if (methodBinding != null && getOutScope(methodBinding.getScope(), null).isContextActive())
+ {// let the XML take precedence
Object result = methodBinding.getMethodBinding().invoke();
- return handleFactoryMethodResult( name, null, result, methodBinding.getScope() );
+ return handleFactoryMethodResult(name, null, result, methodBinding.getScope());
}
- else if ( valueBinding!=null && getOutScope( valueBinding.getScope(), null ).isContextActive() ) //let the XML take precedence
- {
+ else if (valueBinding != null && getOutScope(valueBinding.getScope(), null).isContextActive())
+ { // let the XML take precedence
Object result = valueBinding.getValueBinding().getValue();
- return handleFactoryMethodResult( name, null, result, valueBinding.getScope() );
+ return handleFactoryMethodResult(name, null, result, valueBinding.getScope());
}
- else if ( factoryMethod!=null && getOutScope( factoryMethod.getScope(), factoryMethod.getComponent() ).isContextActive() )
+ else if (factoryMethod != null && getOutScope(factoryMethod.getScope(), factoryMethod.getComponent()).isContextActive())
{
- Object factory = Component.getInstance( factoryMethod.getComponent().getName(), true );
- factoryLock.lock();
- try
+ Object factory = Component.getInstance(factoryMethod.getComponent().getName(), true);
+ Component component = factoryMethod.getComponent();
+ ScopeType scopeResult = getOutScope(factoryMethod.getScope(), factoryMethod.getComponent());
+ ScopeType scopeFactory = factoryMethod.getComponent().getScope();
+ // we need this lock in the following cases: (1) the target scope is
+ // accessed by more than one thread (as we don't want to create the
+ // same object by two threads at the same time for the same scope)
+ // OR (2) the factory is accessed by more than one thread and is
+ // using interceptors (as accessing a factory multiple times might
+ // mess up interceptors). This assumes that (1) the scopes
+ // CONVERSATION, EVENT and PAGE can't be accessed by more than one
+ // thread anyway due to CONVERSATION being locked for the current
+ // thread anyway, and EVENT and PAGE being only short-lived anyway.
+ // (2) a factory that doesn't use injection can be accessed multi
+ // threaded. See JBSEAM-4669/JBSEAM-2419 for the original
+ // discussion.
+ boolean lockingNeeded = ((scopeResult != ScopeType.CONVERSATION && scopeResult != ScopeType.EVENT && scopeResult != ScopeType.PAGE) ||
+ (scopeFactory != ScopeType.CONVERSATION && scopeFactory != ScopeType.EVENT && scopeFactory != ScopeType.PAGE && factoryMethod.getComponent().interceptionEnabled));
+
+ if (lockingNeeded)
{
- // check whether there has been created an instance by another thread while waiting for this function's lock
- if (scope != STATELESS)
+ // synchronize all instances of this component as they might
+ // outject to the same scope (i.e. component factory in EVENT scope,
+ // outjecting to APPLICATION scope).
+ if (scopeResult == ScopeType.CONVERSATION || scopeResult == ScopeType.EVENT || scopeResult == ScopeType.PAGE)
{
- Object value = (scope == null) ? Contexts.lookupInStatefulContexts(name) : scope.getContext().get(name);
- if (value != null)
+ synchronized (factory)
{
- return value;
+ return createInstanceFromFactory(name, scope, factoryMethod, factory);
}
}
-
- if (factory==null)
- {
- return null;
- }
+ // synchronize all instances of this factory as they might
+ // outject to the same scope (i.e. factory in EVENT scope,
+ // outjecting to APPLICATION scope).
else
{
- Object result = factoryMethod.getComponent().callComponentMethod( factory, factoryMethod.getMethod() );
- return handleFactoryMethodResult( name, factoryMethod.getComponent(), result, factoryMethod.getScope() );
+ synchronized (component)
+ {
+ return createInstanceFromFactory(name, scope, factoryMethod, factory);
+ }
}
}
- finally
+ else
{
- factoryLock.unlock();
+ return createInstanceFromFactory(name, scope, factoryMethod, factory);
}
}
else
@@ -2114,6 +2129,30 @@
}
}
+ private static Object createInstanceFromFactory(String name, ScopeType scope, Init.FactoryMethod factoryMethod, Object factory)
+ {
+ // check whether there has been created an instance by another thread
+ // while waiting for this function's lock
+ if (scope != STATELESS)
+ {
+ Object value = (scope == null) ? Contexts.lookupInStatefulContexts(name) : scope.getContext().get(name);
+ if (value != null)
+ {
+ return value;
+ }
+ }
+
+ if (factory == null)
+ {
+ return null;
+ }
+ else
+ {
+ Object result = factoryMethod.getComponent().callComponentMethod(factory, factoryMethod.getMethod());
+ return handleFactoryMethodResult(name, factoryMethod.getComponent(), result, factoryMethod.getScope());
+ }
+ }
+
private static Object handleFactoryMethodResult(String name, Component component, Object result, ScopeType scope) {
// see if a value was outjected by the factory method
Object value = Contexts.lookupInStatefulContexts(name);
@@ -2163,11 +2202,11 @@
}
} catch (Exception e) {
- if (getScope()!=STATELESS) {
- getScope().getContext().remove(name);
- }
+ if (getScope()!=STATELESS) {
+ getScope().getContext().remove(name);
+ }
- throw new InstantiationException("Could not instantiate Seam component: " + name, e);
+ throw new InstantiationException("Could not instantiate Seam component: " + name, e);
}
return instance;
Modified: branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/ConcurrentFactoryTest.java
===================================================================
--- branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/ConcurrentFactoryTest.java 2012-07-03 10:33:05 UTC (rev 14975)
+++ branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/ConcurrentFactoryTest.java 2012-07-03 10:34:31 UTC (rev 14976)
@@ -1,78 +1,123 @@
package org.jboss.seam.test.integration;
-import org.jboss.seam.ScopeType;
+import static org.jboss.seam.ScopeType.APPLICATION;
+
+import java.io.Serializable;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.jboss.seam.annotations.Create;
import org.jboss.seam.annotations.Factory;
+import org.jboss.seam.annotations.In;
import org.jboss.seam.annotations.Name;
-import org.jboss.seam.annotations.Out;
-import org.jboss.seam.annotations.In;
-import org.jboss.seam.contexts.ServletLifecycle;
-import org.jboss.seam.core.Init;
+import org.jboss.seam.annotations.Scope;
+import org.jboss.seam.core.Expressions;
import org.jboss.seam.mock.SeamTest;
-import org.testng.annotations.AfterMethod;
+import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;
-import static org.jboss.seam.ScopeType.APPLICATION;
+// JBSEAM-4669
public class ConcurrentFactoryTest
extends SeamTest
{
- @Override
- protected void startJbossEmbeddedIfNecessary()
- throws org.jboss.deployers.spi.DeploymentException,
- java.io.IOException
+ private volatile boolean exceptionOccured = false;
+ static AtomicInteger testSequence = new AtomicInteger(0);
+
+ private void concurrentFactoryCallTest() throws Exception {
+ new ComponentTest() {
+ @Override
+ protected void testComponents() throws Exception {
+ int myTestSequence;
+ myTestSequence = testSequence.getAndIncrement();
+ if (myTestSequence == 0) {
+ assert "TestString".equals(getValue("#{concurrentFactoryTest.LockHoldingComponent.string}"));
+ } else {
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ assert "TestString".equals(getValue("#{concurrentFactoryTest.dependentString}"));
+ }
+ System.out.println(myTestSequence);
+
+ }
+ }.run();
+ }
+
+ private class ConcurrentFactoryCallTestThread extends Thread
{
- // don't deploy
+ public void run() {
+ try
+ {
+ ConcurrentFactoryTest.this.concurrentFactoryCallTest();
+ }
+ catch (Throwable e)
+ {
+ e.printStackTrace();
+ ConcurrentFactoryTest.this.exceptionOccured = true;
+ }
+ };
}
-
- @Test(threadPoolSize = 2, invocationCount = 2)
+
+ @Test(timeOut=10000)
public void concurrentFactoryCall()
throws Exception
{
- new ComponentTest() {
- @Override
- protected void testComponents() throws Exception {
- assert "slowly created String".equals(getValue("#{concurrentFactoryTest.component.injectedString}"));
- }
- }.run();
+ Thread thread1 = new ConcurrentFactoryCallTestThread();
+ Thread thread2 = new ConcurrentFactoryCallTestThread();
+
+ thread1.start();
+ thread2.start();
+
+ thread1.join();
+ thread2.join();
+
+ assert !exceptionOccured;
}
- @AfterMethod
- @Override
- public void end()
- {
- if (session != null) {
- // Because we run in threads. Only the first thread that finishes ends the session.
- ServletLifecycle.endSession(session);
+ @Name("concurrentFactoryTest.LockHoldingComponent")
+ @Scope(APPLICATION)
+ static public class LockHoldingComponent implements Serializable {
+ @In(value = "concurrentFactoryTest.slowlyCreatedComponent", create = true) SlowlyCreatedComponent slowlyCreatedComponent;
+
+ public String getString() {
+ return (String) Expressions.instance().createValueExpression("#{concurrentFactoryTest.plainFactoryGeneratedString}").getValue();
}
- session = null;
}
- @Name("concurrentFactoryTest.component")
- static public class Component {
- @In(value = "concurrentFactoryTest.slowlyCreatedString") String injectedString;
-
- public String getInjectedString() {
- return injectedString;
+ @Name("concurrentFactoryTest.slowlyCreatedComponent")
+ static public class SlowlyCreatedComponent {
+ @Create
+ public void slowlyCreate() {
+ try {
+ Thread.sleep(1000);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
}
}
+
+ @Name("concurrentFactoryTest.dependentFactory")
+ static public class DependentFactory {
+ @Factory(value = "concurrentFactoryTest.dependentString", scope = APPLICATION, autoCreate = true)
+ public String createString() {
+ return (String) Expressions.instance().createValueExpression("#{concurrentFactoryTest.LockHoldingComponent.string}").getValue();
+ }
+ }
- @Name("concurrentFactoryTest.SlowFactory")
- static public class SlowFactory {
- @Factory(value = "concurrentFactoryTest.slowlyCreatedString", scope = APPLICATION, autoCreate = true)
- public String slowlyCreateString() {
- try
- {
- Thread.sleep(1000);
- return "slowly created String";
- }
- catch (InterruptedException e)
- {
- e.printStackTrace();
- return null;
- }
- }
+ @Name("concurrentFactoryTest.plainFactory")
+ static public class PlainFactory {
+ @Factory(value = "concurrentFactoryTest.plainFactoryGeneratedString", scope = APPLICATION, autoCreate = true)
+ public String createPlainString() {
+ return "TestString";
+ }
}
-
-
+ @AfterClass
+ @Override
+ public void end()
+ {
+ // don't attempt to endSession, as it will block in the deadlocked org.jboss.seam.Component.getInstanceFromFactory lock
+ }
}
Added: branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/FactoryLockTest.java
===================================================================
--- branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/FactoryLockTest.java (rev 0)
+++ branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/FactoryLockTest.java 2012-07-03 10:34:31 UTC (rev 14976)
@@ -0,0 +1,178 @@
+package org.jboss.seam.test.integration;
+
+import javax.ejb.Local;
+import javax.ejb.Remove;
+import javax.ejb.Stateful;
+
+import org.jboss.seam.Component;
+import org.jboss.seam.ScopeType;
+import org.jboss.seam.annotations.Factory;
+import org.jboss.seam.annotations.JndiName;
+import org.jboss.seam.annotations.Name;
+import org.jboss.seam.annotations.Scope;
+import org.jboss.seam.mock.SeamTest;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class FactoryLockTest extends SeamTest
+{
+ private volatile boolean exceptionOccured = false;
+
+ private abstract class TestThread extends Thread {
+ public abstract void runTest() throws Exception;
+
+ @Override
+ public void run()
+ {
+ try
+ {
+ runTest();
+ }
+ catch (Throwable e)
+ {
+ e.printStackTrace();
+ FactoryLockTest.this.exceptionOccured = true;
+ }
+ }
+ }
+
+ // JBSEAM-4993
+ // The test starts two threads, one evaluates #{factoryLock.test.testOtherFactory()} and the other #{factoryLock.testString} 200ms later
+ @Test
+ public void factoryLock()
+ throws Exception
+ {
+ exceptionOccured = false;
+ Thread thread1 = new TestThread() {
+ @Override
+ public void runTest() throws Exception
+ {
+ FactoryLockTest.this.invokeMethod("foo", "#{factoryLock.test.testOtherFactory()}");
+ }
+ };
+
+ Thread thread2 = new TestThread() {
+ @Override
+ public void runTest() throws Exception
+ {
+ Thread.sleep(200);
+ FactoryLockTest.this.getValue("testString", "#{factoryLock.testString}");
+ }
+ };
+
+ thread1.start();
+ thread2.start();
+
+ thread1.join();
+ thread2.join();
+
+ assert !exceptionOccured;
+ }
+
+ // This test is the same as factoryLock test, except it uses the same factory in both threads.
+ @Test
+ public void sameFactoryLock()
+ throws Exception
+ {
+ exceptionOccured = false;
+ Thread thread1 = new TestThread() {
+ @Override
+ public void runTest() throws Exception
+ {
+ FactoryLockTest.this.invokeMethod("testString", "#{factoryLock.test.testSameFactory()}");
+ }
+ };
+
+ Thread thread2 = new TestThread() {
+ @Override
+ public void runTest() throws Exception
+ {
+ Thread.sleep(200);
+ FactoryLockTest.this.getValue("testString", "#{factoryLock.testString}");
+ }
+ };
+
+ thread1.start();
+ thread2.start();
+
+ thread1.join();
+ thread2.join();
+
+ assert !exceptionOccured;
+ }
+
+ private void invokeMethod(final String expected, final String el) throws Exception {
+ new ComponentTest() {
+ @Override
+ protected void testComponents() throws Exception {
+ Assert.assertEquals(expected, invokeMethod(el));
+ }
+ }.run();
+ }
+
+ private void getValue(final String expected, final String el) throws Exception {
+ new ComponentTest() {
+ @Override
+ protected void testComponents() throws Exception {
+ Assert.assertEquals(expected, getValue(el));
+ }
+ }.run();
+ }
+
+ @Local
+ public static interface FactoryLockLocal
+ {
+ public String getTestString();
+ public String testOtherFactory();
+ public String testSameFactory();
+ public void remove();
+ }
+
+
+ @Stateful
+ @Scope(ScopeType.SESSION)
+ @Name("factoryLock.test")
+ //@JndiName("java:global/test/FactoryLockTest$FactoryLockAction")
+ public static class FactoryLockAction implements FactoryLockLocal
+ {
+ public String testOtherFactory() {
+ try
+ {
+ Thread.sleep(500);
+ }
+ catch (InterruptedException e)
+ {
+ e.printStackTrace();
+ }
+ return (String)Component.getInstance("factoryLock.foo", true);
+ }
+
+ // gets instance produced by this component's factory
+ public String testSameFactory() {
+ try
+ {
+ Thread.sleep(500);
+ }
+ catch (InterruptedException e)
+ {
+ e.printStackTrace();
+ }
+ return (String)Component.getInstance("factoryLock.testString", true);
+ }
+
+ @Factory(value="factoryLock.testString", scope=ScopeType.EVENT)
+ public String getTestString() {
+ return "testString";
+ }
+ @Remove
+ public void remove() {}
+ }
+
+ @Name("factoryLock.testProducer")
+ public static class TestProducer {
+ @Factory(value="factoryLock.foo", scope=ScopeType.EVENT)
+ public String getFoo() {
+ return "foo";
+ }
+ }
+}
Modified: branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/testng.xml
===================================================================
--- branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/testng.xml 2012-07-03 10:33:05 UTC (rev 14975)
+++ branches/enterprise/JBPAPP_5_0/src/test/integration/src/org/jboss/seam/test/integration/testng.xml 2012-07-03 10:34:31 UTC (rev 14976)
@@ -10,6 +10,7 @@
<class name="org.jboss.seam.test.integration.JavaBeanEqualsTest" />
<class name="org.jboss.seam.test.integration.NamespaceResolverTest" />
<class name="org.jboss.seam.test.integration.ConcurrentFactoryTest" />
+ <class name="org.jboss.seam.test.integration.FactoryLockTest" />
</classes>
</test>
<test name="Seam Integration Tests - Persistence">
