(T3 only) Multithreading problems possibly due to double-checked locking

• Key: TAPESTRY-806
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Jesse Kuhnert
• Reporter: Nick Westgate
• Component/s: Framework
• Affects Version/s: 3.0.5
• Fix Version/s: 3.0.5

Description

Flawed synchronization code using "double-checked locking" is used in these methods:

(1) org.apache.tapestry.enhance.DefaultComponentClassEnhancer.getEnhancedClass()
(2) org.apache.tapestry.enhance.javassist.EnhancedClassFactory.getObjectType()

Suggested fix for (1) is:

public Class getEnhancedClass(IComponentSpecification specification,
        String className)
{
    synchronized (specification)
    {
        Class result = getCachedClass(specification);

        if (result == null)
        {
            result = getCachedClass(specification);
            if (result == null)
            {
                result = constructComponentClass(specification, className);
                storeCachedClass(specification, result);
            }
        }
    }

    return result;
}

Original bug report, from Michael Prescott ( [e-mail] ) in users list is:

We're coming across an unusual exception that occurs when our
application is under load. Below is an example stack trace, although
the problem can occur while enhancing any of a number of things, so it
doesn't seem specific to one part of our application. The root
exception is always the same, however:
 
  javassist.CannotCompileException: duplicate method: <init>
 
Since it seemed to be related to concurrency, we were reading through
the Tapestry classes along this stack, and found what looks like a
threading problem in DefaultComponentClassEnhancer.java:
 
 public Class getEnhancedClass(IComponentSpecification specification,
String className)
    {
        Class result = getCachedClass(specification);
 
        if (result == null)
        {
            synchronized (this)
            {
                result = getCachedClass(specification);
                if (result == null)
                {
                    result = constructComponentClass(specification,
className);
                    storeCachedClass(specification, result);
                }
            }
        }
 
        return result;
    }
 
This is double-checked locking, which isn't safe given instruction
reordering and thread-specific copies of variables, and all that
multi-CPU magic. This problem is occurring on an 8-CPU server, which
I'm guessing is enough processors to expose the subtle ways in which
double-checked locking can fail.
 
Does this ring any bells for anyone? I realize this is a somewhat
nebulous query.
 
Michael
 
org.apache.tapestry.ApplicationRuntimeException: A code generation error
occured while enhancing class org.apache.tapestry.link.GenericLink.
        at
org.apache.tapestry.enhance.DefaultComponentClassEnhancer.constructCompo
nentClass(DefaultComponentClassEnhancer.java:151)
        at
org.apache.tapestry.enhance.DefaultComponentClassEnhancer.getEnhancedCla
ss(DefaultComponentClassEnhancer.java:97)
        at
org.apache.tapestry.pageload.PageLoader.instantiateComponent(PageLoader.
java:603)
        at
org.apache.tapestry.pageload.PageLoader.createImplicitComponent(PageLoad
er.java:569)
        at
org.apache.tapestry.BaseComponentTemplateLoader.createImplicitComponent(
BaseComponentTemplateLoader.java:295)
        at
org.apache.tapestry.BaseComponentTemplateLoader.process(BaseComponentTem
plateLoader.java:237)
        at
org.apache.tapestry.BaseComponentTemplateLoader.process(BaseComponentTem
plateLoader.java:172)
        at
org.apache.tapestry.BaseComponent.readTemplate(BaseComponent.java:100)
        at
org.apache.tapestry.BaseComponent.finishLoad(BaseComponent.java:135)
        at
org.apache.tapestry.pageload.PageLoader.constructComponent(PageLoader.ja
va:519)
        at
org.apache.tapestry.pageload.PageLoader.createImplicitComponent(PageLoad
er.java:576)
        at
org.apache.tapestry.BaseComponentTemplateLoader.createImplicitComponent(
BaseComponentTemplateLoader.java:295)
        at
org.apache.tapestry.BaseComponentTemplateLoader.process(BaseComponentTem
plateLoader.java:237)
        at
org.apache.tapestry.BaseComponentTemplateLoader.process(BaseComponentTem
plateLoader.java:172)
        at
org.apache.tapestry.BaseComponent.readTemplate(BaseComponent.java:100)
        at
org.apache.tapestry.BaseComponent.finishLoad(BaseComponent.java:135)
        at
org.apache.tapestry.pageload.PageLoader.constructComponent(PageLoader.ja
va:519)
        at
org.apache.tapestry.pageload.PageLoader.createImplicitComponent(PageLoad
er.java:576)
        at
org.apache.tapestry.BaseComponentTemplateLoader.createImplicitComponent(
BaseComponentTemplateLoader.java:295)
        at
org.apache.tapestry.BaseComponentTemplateLoader.process(BaseComponentTem
plateLoader.java:237)
        at
org.apache.tapestry.BaseComponentTemplateLoader.process(BaseComponentTem
plateLoader.java:172)
        at
org.apache.tapestry.BaseComponent.readTemplate(BaseComponent.java:100)
        at
org.apache.tapestry.BaseComponent.finishLoad(BaseComponent.java:135)
        at
org.apache.tapestry.pageload.PageLoader.constructComponent(PageLoader.ja
va:519)
        at
org.apache.tapestry.pageload.PageLoader.createImplicitComponent(PageLoad
er.java:576)
        at
org.apache.tapestry.BaseComponentTemplateLoader.createImplicitComponent(
BaseComponentTemplateLoader.java:295)
        at
org.apache.tapestry.BaseComponentTemplateLoader.process(BaseComponentTem
plateLoader.java:237)
        at
org.apache.tapestry.BaseComponentTemplateLoader.process(BaseComponentTem
plateLoader.java:172)
        at
org.apache.tapestry.BaseComponent.readTemplate(BaseComponent.java:100)
        at
org.apache.tapestry.BaseComponent.finishLoad(BaseComponent.java:135)
        at
org.apache.tapestry.pageload.PageLoader.constructComponent(PageLoader.ja
va:519)
        at
org.apache.tapestry.pageload.PageLoader.loadPage(PageLoader.java:764)
        at
org.apache.tapestry.pageload.PageSource.getPage(PageSource.java:152)
        at
org.apache.tapestry.engine.RequestCycle.getPage(RequestCycle.java:195)
        at
org.apache.tapestry.engine.ActionService.service(ActionService.java:125)
        at
org.apache.tapestry.engine.AbstractEngine.service(AbstractEngine.java:88
9)
        at
org.apache.tapestry.ApplicationServlet.doService(ApplicationServlet.java
:198)
        at
net.exchangesolutions.sn.ui.tapestry.SynchronizedServlet.doService(Synch
ronizedServlet.java:37)
        at
org.apache.tapestry.ApplicationServlet.doGet(ApplicationServlet.java:159
)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:115)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:92)
        at
com.caucho.server.dispatch.ServletFilterChain.doFilter(ServletFilterChai
n.java:99)
        at
net.exchangesolutions.sncore.servlet.StateFilter.doFilter(StateFilter.ja
va:56)
        at
com.caucho.server.dispatch.FilterFilterChain.doFilter(FilterFilterChain.
java:70)
        at
com.caucho.server.webapp.WebAppFilterChain.doFilter(WebAppFilterChain.ja
va:163)
        at
com.caucho.server.dispatch.ServletInvocation.service(ServletInvocation.j
ava:208)
        at
com.caucho.server.hmux.HmuxRequest.handleRequest(HmuxRequest.java:396)
        at
com.caucho.server.port.TcpConnection.run(TcpConnection.java:363)
        at com.caucho.util.ThreadPool.runTasks(ThreadPool.java:490)
        at com.caucho.util.ThreadPool.run(ThreadPool.java:423)
        at java.lang.Thread.run(Thread.java:595)
Caused by: org.apache.tapestry.enhance.CodeGenerationException
        at
org.apache.tapestry.enhance.javassist.ClassFabricator.getByteCode(ClassF
abricator.java:291)
        at
org.apache.tapestry.enhance.javassist.EnhancedClass.createEnhancedSubcla
ss(EnhancedClass.java:130)
        at
org.apache.tapestry.enhance.ComponentClassFactory.createEnhancedSubclass
(ComponentClassFactory.java:336)
        at
org.apache.tapestry.enhance.DefaultComponentClassEnhancer.constructCompo
nentClass(DefaultComponentClassEnhancer.java:143)
        ... 50 more
Caused by: javassist.CannotCompileException: duplicate method: <init>
        at
javassist.bytecode.ClassFile.testExistingMethod(ClassFile.java:488)
        at javassist.bytecode.ClassFile.addMethod(ClassFile.java:472)
        at javassist.CtClassType.addConstructor(CtClassType.java:885)
        at javassist.CtNewClass.addConstructor(CtNewClass.java:55)
        at
javassist.CtNewClass.inheritAllConstructors(CtNewClass.java:96)
        at javassist.CtNewClass.toBytecode(CtNewClass.java:63)
        at javassist.CtClass.toBytecode(CtClass.java:1021)
        at
org.apache.tapestry.enhance.javassist.ClassFabricator.getByteCode(ClassF
abricator.java:279)
        ... 53 more
 

Michael Prescott
direct: 416.646.7062

main: 416.646.7000
fax: [phone]

Exchange Solutions Inc.
250 Yonge Street, 18th Floor
Toronto, ON M5B 2L7
www.exchangesolutions.com

Comments

[Michael Prescott]

The environment is an 8-CPU machine running Solaris 5.8, with JVM 1.5.0_01-b08, under Resin 3.0.14.

[Lee McCallum]

I have found and fixed 7 instances of double-checked locking and changed the synchronization block on 'this' to 'specification' in org.apache.tapestry.enhance.DefaultComponentClassEnhancer.getEnhancedClass(IComponentSpecification specification, String className). This fixes the problem identified in this issue.

[Jesse Kuhnert]

Thanks for the patch, will apply this tomorrow.

[Nick Westgate]

Yay! The squeeky wheel gets oiled! :-)

I've checked the source and inspected the patch.
There are exactly 7 defects in the source.
This patch addresses all of them.

[Jesse Kuhnert]

Patch applied, thanks.