2009/6/25 <ma...@apache.org>: > Author: markt > Date: Wed Jun 24 23:53:11 2009 > New Revision: 788214 > > URL: http://svn.apache.org/viewvc?rev=788214&view=rev > Log: > Add thread safety patch. I haven't voted for it as I need to review it > thoroughly first and it is getting late here. I'll review and vote tomorrow. > > Modified: > tomcat/current/tc5.5.x/STATUS.txt > > + > +* Make access log valves thread safe > + http://people.apache.org/~markt/patches/2009-06-25-AccessLogValve-tc5.patch > + +1: > + -1:
The following my comments are regarding AccessLogValve class only: 1. There is a bug, that sneaked in rev.781753 http://svn.apache.org/viewvc?rev=781753&view=rev That is: the timeZoneNoDST field is never assigned and remains null It was ok in 5.5.27. 2. struct.currentDateString is calculated in two different places I propose to encapsulate that code into AccessDateStruct 3. Invocation of getDate() in invoke() (the only place where getDate() is called): I think that getDate() also can be incapsulated into AccessDateStruct. Actually, I think of the following: 1) In invoke() there is already a variable that stores current time millis: "t2" I propose to replace that getDate() with AccessDateStruct.setDate(t2) that will perform the same job. 2) Change the signature of replace(..., Date, ..) method, and pass a reference to the AccessDateStruct instead of a Date. In the future the replace(..) methods of this class can be changed to accept a StringBuffer, instead of returning a String, but that can be another story. If AccessDateStruct is passed into the replace() method, timeTakenFormatter can be added to it as well. Maybe. 4. The log() method still creates a new Date() once in a second. The Date instance can be stored in some member variable and reused. It is in a synchronized block, so it will be safe. I do not like, that it is the same once-in-a-second synchronized block, that we already avoided once by using those ThreadLocals. Can be addressed separately, though. +1 to the patch regarding AccessLogValve. I'll write about two other classes later, if I find anything. (By the first glance, I have not noticed any problems there). Best regards, Konstantin Kolinko --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
