#361 Possible deadlock caused by improper sync on 2 monitors

[Anonymous]

The issue is simply that FM syncs on the same two monitors on different parts of the code, but not in the same order.
Here are the details and an example of how the deadlock can happen.

Thread 1:
a) ClassBasedModelFactory.get is called: this method acquires a lock on the "cache" variable
b) the method then calls BeansWrapper.introspectClass: this method attempts to acquire a lock on "classCache" variable

Thread 2:
x) BeansWrapper.introspectClass is called: this method acquires a lock on the "classCache" variable
y) the method then calls BeansWrapper.introspectClassInternal that in turn calls ClassBasedModelFactory.clearCache: this method attempts to
acquire a lock on "cache" variable

If the two threads are concurrent and perform the tasks in the following order:
a
x
b
y
we get the deadlock.

[Dániel Dékány]

Note 1: This can only happen if FreeMarker thinks that a class was reloaded, which (ideally...) doesn't happen on production servers.

Note 2: A possible fix would be that if a reloading was detected, then we retry introspectClass, but this time by syncing on the ClassBasedModelFactory instances first, and then see if the reloading is still needed. However, there's possibly another dead-lock situation here, as we have two ClassBasedModelFactory instances with their own locks... what ensures that hey are always locked in the same order as in introspectClassInternal?

[Anonymous]

A possible fix could be to refactor the method ClassBasedModelFactory.get in the following way:

public TemplateModel get(String key) throws TemplateModelException {
TemplateModel model = (TemplateModel)cache.get(key);
if(model == null) {
try {
Class clazz = ClassUtil.forName(key);
model = createModel(clazz);
// This is called so that we trigger the
// class-reloading detector. If there was a class reload,
// the wrapper will in turn call our clearCache method.
wrapper.introspectClass(clazz);
} catch(Exception e) {
throw new TemplateModelException(e);
}
synchronized(cache) {
TemplateModel cachedModel = (TemplateModel)cache.get(key);
if(cachedModel == null) {
cache.put(key, model);
} else {
model = cachedModel;
}
}
return model;
}
}

I can provide a patch if it will simplify the review but I have basically moved the synchronized(cache) block after the call to introspectClass.

[Dániel Dékány]

Indeed, since only "get" uses the BeansWrapper, that is a better approach. (But the cache.get must be put inside its own sync block.)

As of sending patches, it's easier for you if I do this (without looking at what you have just sent...), since otherwise you had to send a signed Contributor License Agreement. However if you plan to do more patches, then it worths the trouble.

[Anonymous]

Daniel, please proceed without my patch if it works for you.

Thanks

[Anonymous]

The patch with the fix discussed in the comments; as suggested by Daniel, also the cache.get call is synchronized for full thread-safe handling.

[Anonymous]

This patch for the BeansWrapper class makes more solid and efficient the way the cache is synchronized; this patch is not required to fix the deadlock but it can significantly improve the performance when BeansWrapper is used by several threads.

[Dániel Dékány]

Fixed in 2.3.20.