DbSqlSession.getUpdatedObjects() and byte arrays

[philipp91]

Hi,

we have an Activiti-based application that loads a large number of tasks and uses getVariablesLocal() to retrieve additional information. However, when used with a larger number of tasks, it crashes due to an ActivitiOptimisticLockingException, although nothing is supposed to be saved to the database, only loading should happen.

I debugged the application and found that getVariablesLocal() causes these updates. The CommandContextInterceptor closes the context, which causes the DbSqlSession to be flushed and the DbSqlSession decides to write all ByteArrayEntities back to the database because of a bug in getUpdatedObjects():
https://github.com/Activiti/Activiti/blob/master/modules/activiti-engine/src/main/java/org/activiti/...
if (!persistentObject.getPersistentState().equals(originalState)) {
Both persistentObject.getPersistentState() and originalStates will be a byte[] in this case, but boxed by Java, so the .equals()-method does not work as expected.

Try this in a Unit-Test:
assertEquals(new byte[] {1}, new byte[] {1})
and it will fail.

Regards,
Philipp

[jbarrez]

Yes, it seems you are correct. That equals will indeed not work in that case.

Would you able to throw together a unit test that demonstrates this, ie how can we 'trigger' the update in the same way you see it?

[philipp91]

I'm sorry I most likely can't make a unit test within reasonable time. I haven't been working much with Activiti yet and I haven't developed the part of our application that calls those methods. So it would even take me some time to get Activiti running in a clean environment …

But maybe these (generalized) code samples can help build a unit test:

Map<String, Object> vars = someRegularValues();
Map<String, String> additionalInfo = getThisFromSomewhere();
vars.put(SOME_CONSTANT, additionalInfo);
runtimeService.setVariablesLocal(processInstanceId, vars);
… // Some time later
// This is the line that causes the crash:
Map<String, Object> vars = runtimeService.getVariablesLocal(processInstanceId);
// So this line does not get executed when too much processes are loaded at a time:
Map<String, String> additionalInfo = (Map<String, String>)vars.get(SOME_CONSTANT);

It is the Map<String, String> in the process variables that is serialized to a byte[] and causes the error.
When executed with a single process, this won't throw any errors, but it will create and commit unnecessary transactions.

[jbarrez]

ok. Created http://jira.codehaus.org/browse/ACT-1524

[philipp91]

Thank you!

[philipp91]

Proposed Solution:
https://github.com/Activiti/Activiti/blob/master/modules/activiti-engine/src/main/java/org/activiti/...
Instead of normal .equals(), use a function like this:
private boolean arrayAwareEquals(Object left, Object right) {
    if (left == null) {
        return right == null;
    } else if (left instanceof Object[]) {
        return Arrays.equals((Object[]) left, (Object[]) right);
    } else {
        return left.equals(right);
    }
}

Another approach would be to use wrapper-objects for all byte-arrays (like this: http://www.java2s.com/Tutorial/Java/0140__Collections/ByteArraywrapsjavabytearraysbytetoallowbytearr...), but that would probably require a lot of code modifications.

[jbarrez]

I've fixed it on master: https://github.com/Activiti/Activiti/commit/ab622919dca574e7ba01c358f8ce5dc8a26f22d5

Could you verify if this fix is what you expected?

[philipp91]

Thank you very much! This is exactly the fix we needed.

[philipp91]

The ByteArray-comparison now works fine.
However, in the case of a HashMap (and also HashSet, which internally uses HashMap), the byte-array will change, so the comparison returns "false", which is actually correct and causes the byte-array to be written to the database although the HashMap hasn't changed.

It should be possible to reproduce it in Activiti 5.12 exactly like I described in my second post, except that it doesn't fail because of the incorrect array comparison, but because the array actually changed even though it shouldn't. Therefore, getVariablesLocal() causes data to be *written* to the database, but it is meant to read only, of course.

// EDIT: This is the statement that detects the "change": https://github.com/Activiti/Activiti/blob/master/modules/activiti-engine/src/main/java/org/activiti/...
// EDIT2: Simple workaround: Do not use hash-based collections. However, Activiti developers might consider checking the variable value and not its serialized form for equality (i.e. use .equals() on the value, in this case this would call HashSet.equals() instead of Arrays.equals(byte[]..)) in order to decide whether to write the value back to the database, or not.

[frederikherema1]

The equals-call on the actual object is not a good solution in all cases, as the deserialized object is kept in memory. The default equals (based in memory-location) will return true every time, even if a member-field is changed. These changes do get picked up by a compare of the actual byte-value of the serialization.

The best solution would be to NOT add a DeserializedObject when variables are fetched only for "read". Not sure if this is an easy fix as the read is done on a low-level.