Do we really need to synchronize on "this" rather than separate some individual locks for better lock granularity?
2009/6/18 <ma...@apache.org> > Author: markt > Date: Thu Jun 18 09:25:00 2009 > New Revision: 785983 > > URL: http://svn.apache.org/viewvc?rev=785983&view=rev > Log: > Expand sync within rotatable block to fix a couple of issues: > - fileDateFormatter is a SimpleDateFormat which is not thread safe > - the rotationLastChecked needs to be volatile to ensure we don't execute > the sync'd block multiple times > Although this is a sync on 'this' in log which gets called for every > request: > - a similar sync occurs in getDate() for every request with minimal > performance impact > - microbenchmarks suggest that a sync on 'this' has similar performance to > using ThreadLocals > > Based on kkolinko's patch for Tomcat 5.5.x > > Note there remains an issue with writing to the log if the log files > happens to be in the process of rotating > > Modified: > tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java > > Modified: tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java > URL: > http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java?rev=785983&r1=785982&r2=785983&view=diff > > ============================================================================== > --- tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java > (original) > +++ tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java Thu > Jun 18 09:25:00 2009 > @@ -295,7 +295,7 @@ > /** > * Instant when the log daily rotation was last checked. > */ > - private long rotationLastChecked = 0L; > + private volatile long rotationLastChecked = 0L; > > /** > * Do we check for log file existence? Helpful if an external > @@ -649,19 +649,21 @@ > // Only do a logfile switch check once a second, max. > long systime = System.currentTimeMillis(); > if ((systime - rotationLastChecked) > 1000) { > - > - rotationLastChecked = systime; > - > - // Check for a change of date > - String tsDate = fileDateFormatter.format(new > Date(systime)); > - > - // If the date has changed, switch log files > - if (!dateStamp.equals(tsDate)) { > - synchronized (this) { > + synchronized(this) { > + if ((systime - rotationLastChecked) > 1000) { > + rotationLastChecked = systime; > + > + String tsDate; > + // Check for a change of date > + tsDate = fileDateFormatter.format(new > Date(systime)); > + > + // If the date has changed, switch log files > if (!dateStamp.equals(tsDate)) { > - close(); > - dateStamp = tsDate; > - open(); > + if (!dateStamp.equals(tsDate)) { > + close(); > + dateStamp = tsDate; > + open(); > + } > } > } > } > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > > -- Sincerely yours and Best Regards, Xie Xiaodong
