Bug 48170 - Unnecessary synchronization by JspFactory.getDefaultFactory contributes to stability problems
Summary: Unnecessary synchronization by JspFactory.getDefaultFactory contributes to st...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 6.0.16
Hardware: Sun Solaris
: P2 major (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-10 18:04 UTC by Earl Nolan
Modified: 2010-02-18 12:13 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Earl Nolan 2009-11-10 18:04:01 UTC
I have a soak test at constant load that is initially stable.  Within the hour, an ever increasing number of blocked threads develops.  The vast majority of threads are in JSP rendering, blocked on JspFactory.getDefaultFactory().

The server eventually crashes.

We are running Java 6.

Upon code inspection, there appears to be no real reason for synchronizing the getDefaultFactory() and setDefaultFactory() as the setter is called only once upon startup when the sub-class loads.

Patching the jar, I tried three other experiments:
1) Removing the synchronized keyword entirely.
2) Locking on an inner static class instead of the JspFactory.class.
3) Using volatile for the static member variable.

Both experiments #1 and #3 showed vastly better stability.  I was able to double the throughput of the server without seeing increasing number of blocked threads.

Experiment #2 yielded the same behavior as the original code.  Thus, no other code
is synchronizing on JspFactory.class.  Rather, there seem to be some sort of contention in the java.lang.Class monitor.

Using volatile would preserve the multi-threading semantics while avoiding contributing to the instability issue.
Comment 1 Sebb 2009-11-11 03:09:43 UTC
(In reply to comment #0)
> I have a soak test at constant load that is initially stable.  Within the hour,
> an ever increasing number of blocked threads develops.  The vast majority of
> threads are in JSP rendering, blocked on JspFactory.getDefaultFactory().
> 
> The server eventually crashes.
> 
> We are running Java 6.
> 
> Upon code inspection, there appears to be no real reason for synchronizing the
> getDefaultFactory() and setDefaultFactory() as the setter is called only once
> upon startup when the sub-class loads.

In which case, the setter should be package-protected?

> Patching the jar, I tried three other experiments:
> 1) Removing the synchronized keyword entirely.
> 2) Locking on an inner static class instead of the JspFactory.class.
> 3) Using volatile for the static member variable.
> 
> Both experiments #1 and #3 showed vastly better stability.  I was able to
> double the throughput of the server without seeing increasing number of blocked
> threads.
> 
> Experiment #2 yielded the same behavior as the original code.  Thus, no other
> code
> is synchronizing on JspFactory.class.  Rather, there seem to be some sort of
> contention in the java.lang.Class monitor.
> 
> Using volatile would preserve the multi-threading semantics while avoiding
> contributing to the instability issue.

Might be worth trying synch. on a private static lock Object instead of an inner class?

If the JspFactory class can be loaded after the JspRuntimeContext class, then JspRuntimeContext could store the factory as a static final field which could be accessed by JspFactory on startup.

Or indeed, do away with setDefaultFactory() and have getDefaultFactory() return the static final value from JspRuntimeContext?
Comment 2 Sebb 2009-11-11 03:13:35 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > I have a soak test at constant load that is initially stable.  Within the hour,
> > an ever increasing number of blocked threads develops.  The vast majority of
> > threads are in JSP rendering, blocked on JspFactory.getDefaultFactory().
> > 
> > The server eventually crashes.
> > 
> > We are running Java 6.
> > 
> > Upon code inspection, there appears to be no real reason for synchronizing the
> > getDefaultFactory() and setDefaultFactory() as the setter is called only once
> > upon startup when the sub-class loads.
> 
> In which case, the setter should be package-protected?

Sorry, that's nonsense - different packages.

> > Patching the jar, I tried three other experiments:
> > 1) Removing the synchronized keyword entirely.
> > 2) Locking on an inner static class instead of the JspFactory.class.
> > 3) Using volatile for the static member variable.
> > 
> > Both experiments #1 and #3 showed vastly better stability.  I was able to
> > double the throughput of the server without seeing increasing number of blocked
> > threads.
> > 
> > Experiment #2 yielded the same behavior as the original code.  Thus, no other
> > code
> > is synchronizing on JspFactory.class.  Rather, there seem to be some sort of
> > contention in the java.lang.Class monitor.
> > 
> > Using volatile would preserve the multi-threading semantics while avoiding
> > contributing to the instability issue.
> 
> Might be worth trying synch. on a private static lock Object instead of an
> inner class?

That might perhaps help.

> If the JspFactory class can be loaded after the JspRuntimeContext class, then
> JspRuntimeContext could store the factory as a static final field which could
> be accessed by JspFactory on startup.
> 
> Or indeed, do away with setDefaultFactory() and have getDefaultFactory() return
> the static final value from JspRuntimeContext?

That's nonsense too.
Comment 3 Earl Nolan 2009-11-11 09:27:26 UTC
The simplest approach is to change the static member variable declaration:

private static volatile JspFactory deflt = null;

and then remove the synchronized keyword on the getter/setter.

This preserves the existing multi-threaded guarantees while not incurring the monitor overhead.  If effect, exchanging a monitor for a memory latch.
Comment 4 Mark Thomas 2010-02-08 14:24:41 UTC
Fixed in trunk and proposed for 6.0.x

Note as a part of the JSP API, the changes permitted are limited.
Comment 5 Mark Thomas 2010-02-18 12:13:33 UTC
This has been fixed in 6.0.x and will be included in 6.0.25 onwards.