Author: alex.guizar(a)jboss.com Date: 2010-05-25 21:02:24 -0400 (Tue, 25 May 2010) New Revision: 6376 Modified: jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/JbpmConfiguration.java jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/ant/DeployProcessTask.java jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/taskmgmt/exe/TaskMgmtInstance.java jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmConfigurationTest.java jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmContextTest.java jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmDefaultConfigTest.java jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/jbpm2630/JBPM2630Test.java Log: JBPM-2630: review and improve warnings logged when closing context in unstructured manner or from another thread check that unstructured context close succeeds despite the warning Modified: jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/JbpmConfiguration.java =================================================================== --- jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/JbpmConfiguration.java 2010-05-25 16:18:06 UTC (rev 6375) +++ jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/JbpmConfiguration.java 2010-05-26 01:02:24 UTC (rev 6376) @@ -543,61 +543,18 @@ } static JbpmConfiguration getCurrentJbpmConfiguration() { - JbpmConfiguration currentJbpmConfiguration = null; List stack = getJbpmConfigurationStack(); - if (!stack.isEmpty()) { - currentJbpmConfiguration = (JbpmConfiguration) stack.get(stack.size() - 1); - } - return currentJbpmConfiguration; + return stack.isEmpty() ? null : (JbpmConfiguration) stack.get(stack.size() - 1); } private static List getJbpmConfigurationStack() { return (List) jbpmConfigurationStacks.get(); } - static void clearJbpmConfigurationStack() { - List configStack = getJbpmConfigurationStack(); - if (configStack != null) { - JbpmConfiguration[] configs = (JbpmConfiguration[]) configStack.toArray(new JbpmConfiguration[configStack.size()]); - for (int f = 0; f < configs.length; f++) { - List contextStack = configs[f].getJbpmContextStack(); - if (contextStack != null) { - JbpmContext[] contexts = (JbpmContext[]) contextStack.toArray(new JbpmContext[contextStack.size()]); - for (int t = 0; t < contexts.length; t++) { - contexts[t].close(); - } - } - assert contextStack.isEmpty() : contextStack; - } - assert configStack.isEmpty() : configStack; - } - } - static void clearInstances() { instances.clear(); } - private void pushJbpmConfiguration() { - getJbpmConfigurationStack().add(this); - } - - private void popJbpmConfiguration() { - List stack = getJbpmConfigurationStack(); - int index = stack.lastIndexOf(this); - if (index == -1) { - log.warn(this + " was not found in thread-local stack;" - + "do not access JbpmContext instances from multiple threads"); - } - else { - if (index != stack.size() - 1) { - log.warn(this + " was not closed in reverse creation order;" - + " check your try-finally clauses around JbpmContext blocks"); - } - // prevent configuration from remaining in the stack, no matter what - stack.remove(index); - } - } - public JbpmContext getCurrentJbpmContext() { ensureOpen(); @@ -610,26 +567,52 @@ } void pushJbpmContext(JbpmContext jbpmContext) { - pushJbpmConfiguration(); + // first push the configuration + getJbpmConfigurationStack().add(this); + // then push the context getJbpmContextStack().add(jbpmContext); } void popJbpmContext(JbpmContext jbpmContext) { - List stack = getJbpmContextStack(); - int index = stack.lastIndexOf(jbpmContext); - if (index == -1) { - log.warn(jbpmContext + " was not found in thread-local stack;" - + " do not access JbpmContext instances from multiple threads"); + boolean threadSafetyFlag = false; + boolean creationOrderFlag = false; + + // first pop the context + List contextStack = getJbpmContextStack(); + int contextIndex = contextStack.lastIndexOf(jbpmContext); + if (contextIndex == -1) { + threadSafetyFlag = true; } else { - if (index != stack.size() - 1) { - log.warn(jbpmContext + " was not closed in reverse creation order;" - + " check your try-finally clauses around JbpmContext blocks"); + if (contextIndex != contextStack.size() - 1) { + creationOrderFlag = true; } // prevent context from remaining in the stack, no matter what - stack.remove(index); + contextStack.remove(contextIndex); } - popJbpmConfiguration(); + + // then pop the configuration + List configStack = getJbpmConfigurationStack(); + int configIndex = configStack.lastIndexOf(this); + if (configIndex == -1) { + threadSafetyFlag = true; + } + else { + if (configIndex != configStack.size() - 1) { + creationOrderFlag = true; + } + // prevent configuration from remaining in the stack, no matter what + configStack.remove(configIndex); + } + + if (threadSafetyFlag) { + log.warn(jbpmContext + " was not closed in the thread that created it;" + + " JbpmContext is not safe for access from multiple threads!"); + } + else if (creationOrderFlag) { + log.warn(jbpmContext + " was not closed in a block-structured manner;" + + " check your try-finally clauses around JbpmContext blocks"); + } } public void startJobExecutor() { Modified: jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/ant/DeployProcessTask.java =================================================================== --- jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/ant/DeployProcessTask.java 2010-05-25 16:18:06 UTC (rev 6375) +++ jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/ant/DeployProcessTask.java 2010-05-26 01:02:24 UTC (rev 6376) @@ -68,8 +68,9 @@ for (int i = 0; i < includedFiles.length; i++) { String fileName = includedFiles[i]; - if (!ArrayUtil.contains(excludedFiles, fileName)) + if (!ArrayUtil.contains(excludedFiles, fileName)) { handleProcessFile(jbpmConfiguration, new File(baseDir, fileName)); + } } } } @@ -77,12 +78,12 @@ private void handleProcessFile(JbpmConfiguration jbpmConfiguration, File processFile) { JbpmContext jbpmContext = jbpmConfiguration.createJbpmContext(); try { - log("parsing process archive " + processFile.getName()); + log("parsing process archive: " + processFile); ProcessDefinition processDefinition = parseProcessArchive(processFile); deployProcessDefinition(processDefinition, jbpmContext); } catch (IOException e) { - log("error reading process archive " + processFile.getName(), e, Project.MSG_ERR); + log("failed to read process archive: " + processFile, e, Project.MSG_ERR); if (failOnError) throw new BuildException(e, getLocation()); } finally { Modified: jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/taskmgmt/exe/TaskMgmtInstance.java =================================================================== --- jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/taskmgmt/exe/TaskMgmtInstance.java 2010-05-25 16:18:06 UTC (rev 6375) +++ jbpm3/branches/jbpm-3.2-soa/modules/core/src/main/java/org/jbpm/taskmgmt/exe/TaskMgmtInstance.java 2010-05-26 01:02:24 UTC (rev 6376) @@ -178,10 +178,11 @@ // evaluate the description if (task != null) { String description = task.getDescription(); - if ((description != null) && (description.indexOf("#{") != -1)) { - Object result = JbpmExpressionEvaluator.evaluate(description, executionContext); + if (description != null) { + String result = (String) JbpmExpressionEvaluator + .evaluate(description, executionContext, String.class); if (result != null) { - taskInstance.setDescription(result.toString()); + taskInstance.setDescription(result); } } } Modified: jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmConfigurationTest.java =================================================================== --- jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmConfigurationTest.java 2010-05-25 16:18:06 UTC (rev 6375) +++ jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmConfigurationTest.java 2010-05-26 01:02:24 UTC (rev 6376) @@ -32,15 +32,17 @@ protected void setUp() throws Exception { super.setUp(); - JbpmConfiguration.clearJbpmConfigurationStack(); JbpmConfiguration.clearInstances(); + } + + protected void tearDown() throws Exception { JbpmConfiguration.setDefaultObjectFactory(null); + super.tearDown(); } public void testSingleton() { JbpmConfiguration.setDefaultObjectFactory(new ObjectFactoryImpl(null, null)); JbpmConfiguration instance = JbpmConfiguration.getInstance(); - assertNotNull(instance); assertSame(instance, JbpmConfiguration.getInstance()); assertSame(instance, JbpmConfiguration.getInstance()); assertSame(instance, JbpmConfiguration.getInstance()); @@ -50,8 +52,13 @@ JbpmConfiguration jbpmConfiguration = JbpmConfiguration.parseXmlString("<jbpm-configuration>" + " <jbpm-context />" + "</jbpm-configuration>"); - assertNotNull(jbpmConfiguration); - assertNotNull(jbpmConfiguration.createJbpmContext()); + JbpmContext jbpmContext = jbpmConfiguration.createJbpmContext(); + try { + assertNotNull(jbpmContext); + } + finally { + jbpmContext.close(); + } } public void testNonExistingContext() { @@ -71,12 +78,20 @@ + " <jbpm-context name='a' />" + " <jbpm-context name='b' />" + "</jbpm-configuration>"); - assertNotNull(jbpmConfiguration); + JbpmContext a = jbpmConfiguration.createJbpmContext("a"); - assertNotNull(a); - JbpmContext b = jbpmConfiguration.createJbpmContext("b"); - assertNotNull(b); - assertNotSame(a, b); + try { + JbpmContext b = jbpmConfiguration.createJbpmContext("b"); + try { + assertNotSame(a, b); + } + finally { + b.close(); + } + } + finally { + a.close(); + } } public void testNonSingletonContextCreation() { @@ -84,11 +99,20 @@ + " <jbpm-context name='a' />" + "</jbpm-configuration>"); assertNotNull(jbpmConfiguration); + JbpmContext a = jbpmConfiguration.createJbpmContext("a"); - assertNotNull(a); - JbpmContext a2 = jbpmConfiguration.createJbpmContext("a"); - assertNotNull(a2); - assertNotSame(a, a2); + try { + JbpmContext a2 = jbpmConfiguration.createJbpmContext("a"); + try { + assertNotSame(a, a2); + } + finally { + a2.close(); + } + } + finally { + a.close(); + } } public void testParseXmlFault() { @@ -123,11 +147,7 @@ private static final long serialVersionUID = 1L; public Object createObject(String name) { - Object o = null; - if ("myproperty".equals(name)) { - o = "mycustomfactoriedvalue"; - } - return o; + return "myproperty".equals(name) ? "mycustomfactoriedvalue" : null; } public boolean hasObject(String name) { Modified: jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmContextTest.java =================================================================== --- jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmContextTest.java 2010-05-25 16:18:06 UTC (rev 6375) +++ jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmContextTest.java 2010-05-26 01:02:24 UTC (rev 6376) @@ -35,36 +35,59 @@ protected void setUp() throws Exception { super.setUp(); - JbpmConfiguration.clearJbpmConfigurationStack(); JbpmConfiguration.clearInstances(); + } + + protected void tearDown() throws Exception { JbpmConfiguration.setDefaultObjectFactory(null); + super.tearDown(); } public void testServices() { JbpmConfiguration jbpmConfiguration = JbpmConfiguration.parseXmlString("<jbpm-configuration>" - + " <jbpm-context name='a' />" - + "</jbpm-configuration>"); - assertNotNull(jbpmConfiguration); - Services s = jbpmConfiguration.createJbpmContext("a").getServices(); - assertNotNull(s); - Services s2 = jbpmConfiguration.createJbpmContext("a").getServices(); - assertNotNull(s2); - assertNotSame(s, s2); + + " <jbpm-context name='a' />" + + "</jbpm-configuration>"); + + JbpmContext a = jbpmConfiguration.createJbpmContext("a"); + try { + Services s = a.getServices(); + + JbpmContext a2 = jbpmConfiguration.createJbpmContext("a"); + try { + Services s2 = a2.getServices(); + assertNotSame(s, s2); + } + finally { + a2.close(); + } + } + finally { + a.close(); + } } public void testJbpmContext() { JbpmConfiguration jbpmConfiguration = JbpmConfiguration.parseXmlString("<jbpm-configuration>" - + " <jbpm-context name='a' />" - + "</jbpm-configuration>"); + + " <jbpm-context name='a' />" + + "</jbpm-configuration>"); + JbpmContext one = jbpmConfiguration.createJbpmContext("a"); - assertNotNull(one); - JbpmContext two = jbpmConfiguration.createJbpmContext("a"); - assertNotNull(two); - assertNotSame(one, two); + try { + + JbpmContext two = jbpmConfiguration.createJbpmContext("a"); + try { + assertNotSame(one, two); + } + finally { + two.close(); + } + } + finally { + one.close(); + } } public static class TestServiceFactory implements ServiceFactory { - private static final long serialVersionUID = 1L; public Service openService() { @@ -76,7 +99,6 @@ } public static class TestService implements Service { - private static final long serialVersionUID = 1L; public void close() { @@ -85,48 +107,67 @@ public void testCustomService() { JbpmConfiguration jbpmConfiguration = JbpmConfiguration.parseXmlString("<jbpm-configuration>" - + " <jbpm-context name='a'>" - + " <service name='test' factory='org.jbpm.JbpmContextTest$TestServiceFactory' />" - + " </jbpm-context>" - + "</jbpm-configuration>"); - Object service = jbpmConfiguration.createJbpmContext("a").getServices().getService("test"); - assertNotNull(service); - assertEquals(TestService.class, service.getClass()); + + " <jbpm-context name='a'>" + + " <service name='test' factory='org.jbpm.JbpmContextTest$TestServiceFactory' />" + + " </jbpm-context>" + + "</jbpm-configuration>"); + JbpmContext jbpmContext = jbpmConfiguration.createJbpmContext("a"); + try { + Object service = jbpmContext.getServices().getService("test"); + assertSame(TestService.class, service.getClass()); + } + finally { + jbpmContext.close(); + } } public void testServiceInFactoryElement() { JbpmConfiguration jbpmConfiguration = JbpmConfiguration.parseXmlString("<jbpm-configuration>" - + " <jbpm-context name='a'>" - + " <service name='test'>" - + " <factory>" - + " <bean class='org.jbpm.JbpmContextTest$TestServiceFactory' />" - + " </factory>" - + " </service>" - + " </jbpm-context>" - + "</jbpm-configuration>"); - Object service = jbpmConfiguration.createJbpmContext("a").getServices().getService("test"); - assertNotNull(service); - assertEquals(TestService.class, service.getClass()); + + " <jbpm-context name='a'>" + + " <service name='test'>" + + " <factory>" + + " <bean class='org.jbpm.JbpmContextTest$TestServiceFactory' />" + + " </factory>" + + " </service>" + + " </jbpm-context>" + + "</jbpm-configuration>"); + JbpmContext jbpmContext = jbpmConfiguration.createJbpmContext("a"); + try { + Object service = jbpmContext.getServices().getService("test"); + assertSame(TestService.class, service.getClass()); + } + finally { + jbpmContext.close(); + } } public void testServiceCaching() { JbpmConfiguration jbpmConfiguration = JbpmConfiguration.parseXmlString("<jbpm-configuration>" - + " <jbpm-context name='a'>" - + " <service name='test' factory='org.jbpm.JbpmContextTest$TestServiceFactory' />" - + " </jbpm-context>" - + "</jbpm-configuration>"); - JbpmContext jbpmContext = jbpmConfiguration.createJbpmContext("a"); - TestService serviceOne = (TestService) jbpmContext.getServices().getService("test"); - assertNotNull(serviceOne); - TestService serviceTwo = (TestService) jbpmContext.getServices().getService("test"); - assertNotNull(serviceTwo); - assertSame(serviceOne, serviceTwo); - jbpmContext = jbpmConfiguration.createJbpmContext("a"); - assertNotSame(serviceOne, jbpmContext.getServices().getService("test")); + + " <jbpm-context name='a'>" + + " <service name='test' factory='" + TestServiceFactory.class.getName() + "' />" + + " </jbpm-context>" + + "</jbpm-configuration>"); + + JbpmContext a = jbpmConfiguration.createJbpmContext("a"); + try { + TestService serviceOne = (TestService) a.getServices().getService("test"); + TestService serviceTwo = (TestService) a.getServices().getService("test"); + assertSame(serviceOne, serviceTwo); + + JbpmContext a2 = jbpmConfiguration.createJbpmContext("a"); + try { + assertNotSame(serviceOne, a2.getServices().getService("test")); + } + finally { + a2.close(); + } + } + finally { + a.close(); + } } public static class CustomLoggingServiceFactory implements ServiceFactory { - private static final long serialVersionUID = 1L; public Service openService() { @@ -138,8 +179,8 @@ } public static class CustomLoggingService implements LoggingService { + private static final long serialVersionUID = 1L; - private static final long serialVersionUID = 1L; ProcessLog processLog; int invocationCount = 0; @@ -154,10 +195,10 @@ public void testCustomLoggingService() { JbpmConfiguration jbpmConfiguration = JbpmConfiguration.parseXmlString("<jbpm-configuration>" - + " <jbpm-context name='a'>" - + " <service name='logging' factory='org.jbpm.JbpmContextTest$CustomLoggingServiceFactory' />" - + " </jbpm-context>" - + "</jbpm-configuration>"); + + " <jbpm-context name='a'>" + + " <service name='logging' factory='" + CustomLoggingServiceFactory.class.getName() + "' />" + + " </jbpm-context>" + + "</jbpm-configuration>"); CustomLoggingService customLoggingService = null; MessageLog messageLog = null; @@ -165,6 +206,7 @@ JbpmContext jbpmContext = jbpmConfiguration.createJbpmContext("a"); try { customLoggingService = (CustomLoggingService) jbpmContext.getServices().getLoggingService(); + messageLog = new MessageLog("blablabla"); ProcessInstance processInstance = new ProcessInstance(new ProcessDefinition()); processInstance.getLoggingInstance().addLog(messageLog); @@ -174,13 +216,13 @@ jbpmContext.close(); } assertEquals(messageLog, customLoggingService.processLog); - assertEquals(2, customLoggingService.invocationCount); // count process instance create log + assertEquals(2, customLoggingService.invocationCount); } public static class TestSaveOperation implements SaveOperation { + private static final long serialVersionUID = 1L; static int invocationCount = 0; - private static final long serialVersionUID = 1L; public void save(ProcessInstance processInstance, JbpmContext jbpmContext) { invocationCount++; @@ -189,22 +231,20 @@ public void testSaveOperation() { JbpmConfiguration jbpmConfiguration = JbpmConfiguration.parseXmlString("<jbpm-configuration>" - + " <jbpm-context name='default.jbpm.context'>" - + " <save-operations>" - + " <save-operation class='org.jbpm.JbpmContextTest$TestSaveOperation' />" - + " </save-operations>" - + " </jbpm-context>" - + "</jbpm-configuration>"); + + " <jbpm-context name='default.jbpm.context'>" + + " <save-operations>" + + " <save-operation class='" + TestSaveOperation.class.getName() + "' />" + + " </save-operations>" + + " </jbpm-context>" + + "</jbpm-configuration>"); + TestSaveOperation.invocationCount = 0; - TestSaveOperation.invocationCount = 0; JbpmContext jbpmContext = jbpmConfiguration.createJbpmContext(); - try { + assertEquals(0, TestSaveOperation.invocationCount); - assertEquals(0, TestSaveOperation.invocationCount); jbpmContext.save(new ProcessInstance()); assertEquals(1, TestSaveOperation.invocationCount); - } finally { jbpmContext.close(); @@ -213,24 +253,22 @@ public void testSaveOperationInBeanElement() { JbpmConfiguration jbpmConfiguration = JbpmConfiguration.parseXmlString("<jbpm-configuration>" - + " <jbpm-context name='default.jbpm.context'>" - + " <save-operations>" - + " <save-operation>" - + " <bean class='org.jbpm.JbpmContextTest$TestSaveOperation' />" - + " </save-operation>" - + " </save-operations>" - + " </jbpm-context>" - + "</jbpm-configuration>"); + + " <jbpm-context name='default.jbpm.context'>" + + " <save-operations>" + + " <save-operation>" + + " <bean class='" + TestSaveOperation.class.getName() + "' />" + + " </save-operation>" + + " </save-operations>" + + " </jbpm-context>" + + "</jbpm-configuration>"); + TestSaveOperation.invocationCount = 0; - TestSaveOperation.invocationCount = 0; JbpmContext jbpmContext = jbpmConfiguration.createJbpmContext(); - try { + assertEquals(0, TestSaveOperation.invocationCount); - assertEquals(0, TestSaveOperation.invocationCount); jbpmContext.save(new ProcessInstance()); assertEquals(1, TestSaveOperation.invocationCount); - } finally { jbpmContext.close(); Modified: jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmDefaultConfigTest.java =================================================================== --- jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmDefaultConfigTest.java 2010-05-25 16:18:06 UTC (rev 6375) +++ jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/JbpmDefaultConfigTest.java 2010-05-26 01:02:24 UTC (rev 6376) @@ -1,10 +1,8 @@ package org.jbpm; - public class JbpmDefaultConfigTest extends AbstractJbpmTestCase { - // this test should be ran without jbpm.cfg.xml on the - // classpath. + // this test should be run without jbpm.cfg.xml on the classpath public void testWithoutJbpmCfgXml() { JbpmConfiguration.getInstance().createJbpmContext().close(); } Modified: jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/jbpm2630/JBPM2630Test.java =================================================================== --- jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/jbpm2630/JBPM2630Test.java 2010-05-25 16:18:06 UTC (rev 6375) +++ jbpm3/branches/jbpm-3.2-soa/modules/core/src/test/java/org/jbpm/jbpm2630/JBPM2630Test.java 2010-05-26 01:02:24 UTC (rev 6376) @@ -50,7 +50,7 @@ super.tearDown(); } - public void testPopJbpmConfiguration() { + public void testBlockStructuredContextClose() { JbpmContext testContext1 = testConfiguration.createJbpmContext(); try { JbpmContext noLogContext = noLogConfiguration.createJbpmContext(); @@ -75,4 +75,20 @@ testContext1.close(); } } + + public void testUnstructuredContextClose() { + JbpmContext testContext = testConfiguration.createJbpmContext(); + assertSame(testConfiguration, + JbpmConfigurationTestHelper.getCurrentJbpmConfiguration()); + + JbpmContext noLogContext = noLogConfiguration.createJbpmContext(); + assertSame(noLogConfiguration, + JbpmConfigurationTestHelper.getCurrentJbpmConfiguration()); + + // this will emit a block-structured warning, but still succeed + testContext.close(); + noLogContext.close(); + + assertNull(JbpmConfigurationTestHelper.getCurrentJbpmConfiguration()); + } }