Author: manaRH Date: 2012-07-03 04:57:58 -0400 (Tue, 03 Jul 2012) New Revision: 14973 Added: branches/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/FactoryLockTest.java Modified: branches/community/Seam_2_2/src/main/org/jboss/seam/Component.java branches/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/ConcurrentFactoryTest.java branches/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/testng.xml Log: backported JBSEAM-4993, JBSEAM-4861 to Seam 2.2 Modified: branches/community/Seam_2_2/src/main/org/jboss/seam/Component.java =================================================================== --- branches/community/Seam_2_2/src/main/org/jboss/seam/Component.java 2012-07-02 12:59:07 UTC (rev 14972) +++ branches/community/Seam_2_2/src/main/org/jboss/seam/Component.java 2012-07-03 08:57:58 UTC (rev 14973) @@ -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,7 +2054,7 @@ 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; } @@ -2066,45 +2063,62 @@ 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); + 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) + // Only one factory instance can access result scope + // CONVERSATION / EVENT / PAGE anyway due to + // the locking of the conversation. + 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 (factory.getClass()) + { + return createInstanceFromFactory(name, scope, factoryMethod, factory); + } } } - finally + else { - factoryLock.unlock(); + return createInstanceFromFactory(name, scope, factoryMethod, factory); } } else @@ -2114,6 +2128,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 +2201,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/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/ConcurrentFactoryTest.java =================================================================== --- branches/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/ConcurrentFactoryTest.java 2012-07-02 12:59:07 UTC (rev 14972) +++ branches/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/ConcurrentFactoryTest.java 2012-07-03 08:57:58 UTC (rev 14973) @@ -2,74 +2,122 @@ 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.contexts.ServletLifecycle; +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; + +// 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/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/FactoryLockTest.java =================================================================== --- branches/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/FactoryLockTest.java (rev 0) +++ branches/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/FactoryLockTest.java 2012-07-03 08:57:58 UTC (rev 14973) @@ -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/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/testng.xml =================================================================== --- branches/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/testng.xml 2012-07-02 12:59:07 UTC (rev 14972) +++ branches/community/Seam_2_2/src/test/integration/src/org/jboss/seam/test/integration/testng.xml 2012-07-03 08:57:58 UTC (rev 14973) @@ -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">