#43 Non threadsafe use of TypeTransformerMap during serialize()

[David Citron]

Although the JavaDocs for JSONSerializer indicate that serialize() can be used concurrently, a bug in the TypeTransformerMap area makes it impossible to even use separate instances of `JSONSerializer`` from different threads at the same time.

JSONSerializer contains a member variable typeTransformerMap which is a reference to a child map that's initialized via TransformerUtil.getDefaultTypeTransformers().

The TransformerUtil.getDefaultTypeTransformers() method returns a static instance to a "parent" TypeTransformerMap which is effectively a global variable.

The child map (which contains the reference to the parent map) is assigned to a thread-local JSONContext during serialize(). The JSONContext.transform() method then attempts to get a transformer from this map (TypeTransformerMap.getTransformer()). That method walks up to the parent and ultimately modifies the global parent map, meaning that two threads can end up modifying this global HashMap concurrently, causing data problems.

These problems typically manifest as a NullPointerException as follows:

Caused by: java.lang.NullPointerException
    at flexjson.JSONContext.transform(JSONContext.java:73)
    at flexjson.JSONSerializer.serialize(JSONSerializer.java:377)
    at flexjson.JSONSerializer.serialize(JSONSerializer.java:235)
    ...

Interestingly, there's a comment in TypeTransformerMap.updateTransformers() indicating knowledge of this issue ("// only make changes to the child TypeTransformerMap"). However, that method does not prevent updates to the parent map.

[Brandon Goodin]

I think you are using an old version of FlexJSON that has a major bug in it where the TypeTransformerMap.parentTransformerMap was being altered.

That being said, reviewing back through the code I do see that I am passing in the the TypeTransformerMap on the JSONSerializer to the JSONContext which in turn makes unsynchronized changes to the child map. I'm pretty sure this is a problem. Some of this code takes a little while for me to revisit and refresh myself.

So, in summary, I think you are using an old version of FlexJSON. The newest version does not suffer from the major issue of changing the static parent retrieved from TransformerUtil.getDefaultTypeTransformers();

But, I will look into the child transformer map that is passed in from the JSONSerializer to the JSONContext and subsequently changed unsynchronized.

Following is a link and pom snippet for the newest FlexJSON:

http://repo2.maven.org/maven2/net/sf/flexjson/flexjson/3.1/

<dependency>
    <groupId>net.sf.flexjson</groupId>
    <artifactId>flexjson</artifactId>
    <version>3.1</version>
</dependency>

UPDATE: TypeTransformerMap extends ConcurrentHashMap to allow for concurrent modification from multiple threads. I think things are safe.

Please let me know if this resolves your issue.

[David Citron]

Yes! We're using 2.1. We tried to update to 3.0 a while ago, but encountered [#42]
Is that bug fixed in version 3.1?

[Brandon Goodin]

I am not sure if that bug is fixed. Charlie is king of the Deserialization. Charlie? Maybe you should bump that bug report. I would expect it could be fixed pretty quickly. If I get a bit more time I'll try to take a look into it.

[Charlie Hubbard]

I remember this defect and I remember looking at it, but I don't remember if I fixed it or not. I'll have to look at it again to see. I seem to remember it being more complicated than it first appeared.

[David Citron]

Noting here that [#42] is not fixed in 3.1. Therefore, until [#42] is fixed, we do not have a working FlexJSON version that we can use.

[Brandon Goodin]

I'll see what I can do about patching 2.1 with the thread safe changes and getting a release out that you can use.

[David Citron]

Thanks!! That would be great!

For now, we just made a one-line change to TypeTransformerMap in our local environment to stop modifying the parent map (we added the "&& parentTransformerMap != null" clause below):

private Transformer updateTransformers(Class key, Transformer transformer) {
    // only make changes to the child TypeTransformerMap
    if (transformer != null && parentTransformerMap != null) {
        put(key, transformer);
    }
    return transformer;
}

I realize that this change skips the caching optimization in the TypeTransformerMap, forcing a potential walk up the class hierarchy for each serialized class in each new JSONSerializer instantiation, but the performance difference wasn't measurable in our system so I figured it was an acceptable change until we could get a better FlexJSON patch :-)

[Brandon Goodin]

Sorry to cross post here. But, since applying this change to the 2.x codebase hinges on the state of issue #42 and Charlie is saying issue #42 will not be fixed I don't really think applying it to the 2.1 codebase will help. I'm going to avoid applying the patch to 2.1 unless issue #42 is accepted and delayed. Sorry for the runaround on this.