Thread safety issue

[antlr]

Thread safety issue

[sharwell]

To be absolutely safe, create one instance of your STGroup for each thread. In Java, you could use ThreadLocal<T> to initialize your group on-demand (by overriding initialValue() to load the group).

[bedla]

Thx for response. I know about ThreadLocal, but it has many issues while using in servlet container (memory leaks vs. war redeployment vs. somebody forgot to clear TL after http servlet request).
It would be nice to have immutable version of template after parsing/compiling from source code. Now I will try to preload template string to eliminate I/O operations.

[paulmillar]

Hi,

My work-around is to explicitly call getInstanceOf after creating the STGroupFile object. This will trigger the loading by a single thread (so, no race) and will populate the Map with the parsed CompiledST objects.

There's still the possibility that a later thread will not see the updated alreadyLoaded value and attempt to parse the file; however, that (while noisy) should be harmless, beyond a performance hit. Fixing that part in StringTemplate should be easy: just mark alreadyLoaded field member as volatile.

After initialising, getInstanceOf builds an ST object from the Map's CompiledST entry. The Map is thread-safe, so getInstanceOf should be fine and subsequent operations (setting the attributes and rendering the result) should operate within the ST object. I'm assuming (but haven't checked) that the CompiledST object is immutable.

I think there's a fair number of extra features that we're not using, so it's possible that other people will still suffer from race conditions, but I think we can get away without synchronising calls to getInstanceOf.

My advise (admittedly without having spent too much time thinking about it :) would be to get rid of the lazy load and either load on construction or (better) have an explicit 'load' or 'init' method that triggers the parsing of the templates. Attempts to use the STGroup before load/init should throw IllegalStateException (or similar), as would calling load/init more than once. It's the callers responsibility to call load/init correctly.

HTH,

Paul.

[seojiwon]

Also, In ST.java, there seems to be a couple of places that modifies impl object (which is a CompiledST instance).

1. ST.java:inspect(...): impl.nativeGroup.setListener (probably this is only for debugging, so doesn't matter??)
2. ST.java: add(String name, Object value) : calling impl.addArg():
   Inside ST::addArg, it lazily creates a (synchronized) map, but potentially, two threads may each creates two instances of a map, and add args in each instance. Either the method should be synchronized (but then all the methods that access formalArguments should also be synchronized), or create the map instance in the constructor, before the CompiledST object is added to the templates map in the STGroup (so, don't do lazy initialization).

[parrt]

Hi. Thanks. I'll look into this when I get a chance.

On Mon, Aug 12, 2013 at 3:10 PM, seojiwon [email protected] wrote:

We use StringTemplate in multi-threaded configuration, where we create a
single STGroup instance, and multiple thread calls group.getInstanceOf and
render.

The problem is we (sometimes) get "attribute XYZ isn't defined" error,
although the attribute XYZ (which is a map/dictionary template) is clearly
defined in an imported template.

My guess is that StringTemplate has thread safety issue when (but not
limited to) a dictionary that is defined in an imported template file is
accessed.

I went over the source code and found a few places that potentially have
thread safety issue.

Lazy initialization of STGroup (STGroupFile) object. STGroupFile calls
load() method lazily when necessary, so multiple thread can simultaneously
initialize the object (which may or may not be a problem). I think probably
load() should be synchronized.
2.

CompiledST object is being modified after putting into the
synchronized 'templates' dictionary.
3.

For example, in STGroup.java in defineRegion method
code.defineArgDefaultValueTemplates(this) and
code.defineImplicitlyDefinedTemplates(this) is called after the invocation
of the rawDefineTemplate method, which puts CompiledST object into the
synchronized map. Thus another thread may see an incomplete CompiledST
object.

I did not check all the code, so there might be other places with thread
safety issues. I made STGroup.getInstanceOf, STGroup.getEmbeddedInstanceOf,
and ST.render to be synchronized with respect to the STGroup instance, and
the attribute error went away.
I suggest that those three methods to be synchronized; but also someone
should go over all the code and check if any other thread-safety issue is
there. Any code that does lazy evaluation should be examined for thread
safety.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/61
.

Dictation in use. Please excuse homophones, malapropisms, and nonsense.

[sharwell]

To reduce (but not eliminate) the chance of threading problems, I am marking a select group of methods as synchronized for the 4.0.8 release. (Edit: I am deferring this issue. See below.) However, the thread safety of ST4 as a whole is not conclusively tested and is known to have problems in some cases. For a multi-threaded application, you should create one instance of your STGroup for each thread in the application, and use the appropriate one for the current thread when rendering templates.

[bedla]

Hi, could you specifiy how to use ST to be safe with threads. I mean, is it safe to create STGroup instance, call load() and than share it among threads (in webapp env, considering IO ops)? And after that when I want to call template just call STGroup.getInstanceOf(...). Thx

[parrt]

Once it's loaded, it shouldn't change at all for getInstanceOf, should it? W/o looking at code, it seems safe. Adding in multiple threads would be bad.

[sharwell]

Unfortunately, I don't have the time to rigorously evaluate the thread safety of ST. There are a few cases that are clearly not safe for multithreaded use, even after the group is loaded. Based on this, I wouldn't assume that any template instances created by a single STGroup can be used concurrently in an application.

I'll work to implement a thread-safe version of ST4, but it won't appear before version 4.1 at minimum.

[sharwell]

For release 4.0.8, I'm going to leave the synchronization code as-is.

[bedla]

Ok, thx for info.

[paulmillar]

I found a problem that seems to be from a race-condition from the lazy loading.

What I believe is happening is that more than one thread calling getInstanceOf (in STGroup) concurrently. Within STGroupFile, the alreadyLoaded field member controls the lazy load. This is not declared volatile, so the visibility between different threads after update is not guaranteed, resulting in potential multiple initialisations -- while a problem, I don't believe this causes the problem I'm seeing. Rather, the problem seems to be because alreadyLoaded is set to true before the initialisation completes.

Consider two threads calling getInstanceOf at the same time. The first race is to see whether both threads or just one will parse the file. Assume one parses the file and the other doesn't (believing the file is already loaded), the next race starts. The thread parsing the file will attempt to put the CompiledST into the templates Map, while the thread that doesn't parse the file will attempt to put NOT_FOUND_ST constant into the Map.

Since the thread that is parsing the file will likely take longer, lets say the other thread wins the race and puts the NOT_FOUND_ST into the Map. After parsing, the thread will attempt to put the CompiledST into the Map, which will fail with a "redefinition of template" error message.

The thread that parsed the file should return a non-null value getInstanceOf, but all other attempts for this template will return null.

[parrt]

Hi. I believe this is similar to another issue related to thread safety. I believe that it is not safe to access the same template group from multiple threads without synchronizing the access yourself. I don't think we've been particularly clear about that. Sorry. I'm leaving this open because it might be something we can easily fix. In the meantime, can you put a synchronized block around get instance of?

[paulmillar]

Potential solution:

#83

HTH,

Paul.

[sharwell]

@paulmillar For StringTemplate 4.0.x, you should not use the same STGroup instance in multiple threads concurrently. You could either create an STGroup instance for each thread that needs one (each instance tied to a specific thread), or use an object pool capable of assigning an STGroup instance to a particular thread, and then "returning" it to the object pool where it is no longer associated with a particular thread (improving performance for applications that do not use dedicated long-running render threads).

The concurrency issues need to be thoroughly evaluated throughout ST4 before I'm comfortable changing this recommendation, and implemented in a manner designed with concurrent rendering in mind from the start. Your comments definitely help with this analysis, but they are likely incomplete so users in other scenarios could still hit unexpected behavior when using ST4 in concurrent applications.

[parrt]

Hi Paul, sorry for the long delay. Thanks very much for raising this issue and your potential solution. As Sam says, however, I'm a bit nervous with the addition of synchronized. I'm not convinced it will solve the problem without looking at it very carefully. It's often the case that there are multiple methods that need to execute as a transaction rather than simply providing two methods from executing at the same time. A simple solution is for you to create a proxy object that locks. I can usually get intellij to generate a proxy object that delegates. Consequently, I'm closing this issue. thanks.

[hartmut27]

Just as a side node - you need an threadsafe singleton mechanism, to prevent multithreading issues while initializing the STGroup. I've chosen Google Guava's micro caching Suppliers.memoize.
By the way - I did not encounter problems afterwards sharing the intitiatlized STGroup beteween threads.

Supplier<STGroup> stGroupSupplier = Suppliers.memoize(
  new Supplier<STGroup>() {
    @Override
    public STGroup get() {
      try {
        return new STGroup(...)
      }
   }
}

then

stGroupSupplier.get()

from the Guava javadoc: "The returned supplier is thread-safe. The delegate's get() method will be invoked at most once."

[huangyg11]

@hartmut27 The problem here is the lazy loading( and paring) of the template file, not the initializing of STGroup. So your code may need to include the loading of template file to work.