Error running 5.12 unit tests with Java 7

[brianshowers]

Due to the issue with saving TaskEntry objects (originally mentioned here), we are currently using a forked and patched version of Activiti v5.11.  I wanted to upgrade to v5.12, so I was going to reapply my patch on top of the 5.12 tag and rebuild.  However, I've started getting an error running the Activiti unit tests in 5.12 under a Java 7 JVM.

Specifically, what I see is org.activiti.examples.bpmn.scripttask.ScriptTaskTest::testAutoStoreVariables() fails with the following exception:

Environment: Windows 7, Java 1.7.0_17

——————————————————————————-
Test set: org.activiti.examples.bpmn.scripttask.ScriptTaskTest
——————————————————————————-
Tests run: 4, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.193 sec <<< FAILURE!
testAutoStoreVariables(org.activiti.examples.bpmn.scripttask.ScriptTaskTest)  Time elapsed: 0.075 sec  <<< ERROR!
org.activiti.engine.ActivitiException: couldn't find a variable type that is able to serialize sun.org.mozilla.javascript.internal.Undefined@30baae4a
   at org.activiti.engine.impl.variable.DefaultVariableTypes.findVariableType(DefaultVariableTypes.java:62)
   at org.activiti.engine.impl.persistence.entity.VariableScopeImpl.createVariableInstance(VariableScopeImpl.java:359)
   at org.activiti.engine.impl.persistence.entity.VariableScopeImpl.createVariableLocal(VariableScopeImpl.java:292)
   at org.activiti.engine.impl.persistence.entity.VariableScopeImpl.createVariableLocal(VariableScopeImpl.java:279)
…
   at org.activiti.engine.impl.TaskServiceImpl.complete(TaskServiceImpl.java:161)
   at org.activiti.examples.bpmn.scripttask.ScriptTaskTest.testAutoStoreVariables(ScriptTaskTest.java:76)
   at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
   at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   at java.lang.reflect.Method.invoke(Method.java:601)
   at junit.framework.TestCase.runTest(TestCase.java:176)
   at org.activiti.engine.impl.test.PvmTestCase.runTest(PvmTestCase.java:65)
   at junit.framework.TestCase.runBare(TestCase.java:141)
   at org.activiti.engine.impl.test.AbstractActivitiTestCase.runBare(AbstractActivitiTestCase.java:86)
   at junit.framework.TestResult$1.protect(TestResult.java:122)
   at junit.framework.TestResult.runProtected(TestResult.java:142)
   at junit.framework.TestResult.run(TestResult.java:125)
   at junit.framework.TestCase.run(TestCase.java:129)
   at junit.framework.TestSuite.runTest(TestSuite.java:255)
   at junit.framework.TestSuite.run(TestSuite.java:250)
   at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:84)
   at org.apache.maven.surefire.junit4.JUnit4TestSet.execute(JUnit4TestSet.java:35)
   at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:146)
   at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:97)
   at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
   at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   at java.lang.reflect.Method.invoke(Method.java:601)
   at org.apache.maven.surefire.booter.ProviderFactory$ClassLoaderProxy.invoke(ProviderFactory.java:103)
   at com.sun.proxy.$Proxy0.invoke(Unknown Source)
   at org.apache.maven.surefire.booter.SurefireStarter.invokeProvider(SurefireStarter.java:145)
   at org.apache.maven.surefire.booter.SurefireStarter.runSuitesInProcess(SurefireStarter.java:87)
   at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:69)
‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍

However, if I run the same test using a Java 1.6.0_25 JVM, it passes!

I spent some time stepping through the test in the debugger, and it seems to come down to a change in the sun.org.mozilla.javascript.internal.Undefined implementation.  In the Java 6 implemenation, that class implements Serializable.  So, org.activiti.engine.impl.variable.DefaultVariableTypes::findVariableType() is able to map Undefined to Serializable.

However, in Java 1.7, it appears that sun.org.mozilla.javascript.internal.Undefined does not implement Serializeable.  So, no valid type can be found and an exception is thrown.

I haven't been able to track down any source code for the Undefined class, so it's a bit hard for me to tell for certain, but this test fails for me in Java 7 and passes in Java 6:

  public void testUndefinedType() {
      boolean serializable = (Undefined.instance instanceof Serializable);
      assertTrue(serializable);
  }
‍‍‍‍‍‍

While it's not that big of deal if I can't run the Activiti tests in the Java 1.7 VM, I think the issue might be bigger than that.  I think that Activiti won't be able to save script variables of an undefined type in a Java 7 JVM.

[jbarrez]

Hmmm that sure doesn't sound good …

I don't know how we could force the 'undefined' variable to be coerced to a valid java type without depending on an internal sun.org.xxx class..
Maybe it is time to switch to Rhino (https://developer.mozilla.org/en-US/docs/Rhino) as javascript impl for Activiti? The default JDK javascript support has always been wonky and unstable …

[brianshowers]

@Joram, thanks for the quick action.  I have to say that Activiti is, by far, the best open source community that I've worked with.  I'm extremely impressed with how quickly you follow up and take action.  Most communities tend to de-prioritize any bugs that don't impact their personal code paths.

That said, if there is a way for folks like me to get more involved as a committer, I'm always happy to provide back the fixes and investigations that we find.

[jbarrez]

@Joram, thanks for the quick action. I have to say that Activiti is, by far, the best open source community that I've worked with. I'm extremely impressed with how quickly you follow up and take action. Most communities tend to de-prioritize any bugs that don't impact their personal code paths.

Thanks!

That said, if there is a way for folks like me to get more involved as a committer, I'm always happy to provide back the fixes and investigations that we find.

We're pretty liberal with committing stuff. The easiest is just creating a pull request on github. That way, it's very easy for us to incorporate any fixes people have made. If you need any directions with how to do that, let me know.
But an investigation like the one you did in this thread is of course also a *very* valuable contribution!

That being said, we still don't have a solution for the jdk 1.7.0_17 problem (and apparently latest jdk 6 now too … ). We're really pondering what would be the best (long-term) solution for this.

[jbarrez]

Some updates: we discussed it this morning in the team and decided we're gonna make the auto storage of variables not the default anymore. If you want to store something, you need to explicitly call the setVariable method.

For backwards compatibility, we'll keep the 'autoStoreVariables' property so people can set it to true if they really depend on it.

[brianshowers]

Won't the problem still exist if you try to save a variable of type Undefined?  It will be less prevalent since autoSaving will default to off, but I would think that any explicit save operation would result in the same stack trace.  Seems like you would also need to give the variables a specific type as well.

That said, I have to admit that we don't actually use script blocks in our diagrams, so I'm certainly no expert in that area.

[jbarrez]

Won't the problem still exist if you try to save a variable of type Undefined? It will be less prevalent since autoSaving will default to off, but I would think that any explicit save operation would result in the same stack trace. Seems like you would also need to give the variables a specific type as well.

No, when you do execution.setVariable(), the script engine will convert it to a Java type. However, the auto storage does not do that. And we can't do it either cause it are all internal classes of the JDK. So as long as you explicitely call setVariable, all is fine. Also note it is only needed for Javascript (groovy still works ok).

I adapted the default behavior and changed just now on master: https://github.com/Activiti/Activiti/commit/5b1982e719e448b91f2c49d05afbd58f62683666

[brianshowers]

Got it.  Makes total sense.