Author: markt
Date: Sun Dec 5 19:22:19 2010
New Revision: 1042421
URL: http://svn.apache.org/viewvc?rev=1042421&view=rev
Log:
Update patch.
Modified:
tomcat/tc6.0.x/trunk/STATUS.txt
Modified: tomcat/tc6.0.x/trunk/STATUS.txt
URL:
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1042421&r1=1042420&r2=1042421&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Sun Dec 5 19:22:19 2010
@@ -204,32 +204,40 @@ PATCHES PROPOSED TO BACKPORT:
* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=50201
Handle ROOT webapp redployment, host start/stop etc for default access log
- http://people.apache.org/~markt/patches/2010-12-02-bug50201-tc6.patch
+ http://people.apache.org/~markt/patches/2010-12-05-bug50201-tc6.patch
+1: markt
-1: kkolinko:
1) A typo: in "checkHost = ((ContainerBase) context).started;"
- should be s/checkHost/checkContext/
+ should be s/checkHost/checkContext/
+ markt: Fixed in updated patch above
2) Possible NPEs in StandardEngine.logAccess(). With the old code
- the defaultAccessLog field can be checked once, and we know that
- if it is already not-null it will never become null. That is no
- longer the case and subsequent accesses to that volatile field can result
in NPE.
- One needs to keep a local copy of that field.
+ the defaultAccessLog field can be checked once, and we know that
+ if it is already not-null it will never become null. That is no
+ longer the case and subsequent accesses to that volatile field can result
+ in NPE. One needs to keep a local copy of that field.
+ markt: Fixed in updated patch above
3) addPropertyChangeListener(l). If we are only interested in "defaultHost"
- property change, shouldn't we add the listener to StandardEngine?
- I think that there is no need to add it to Context or Host (and
- are they notified about that change at all?).
+ property change, shouldn't we add the listener to StandardEngine?
+ I think that there is no need to add it to Context or Host (and
+ are they notified about that change at all?).
+ markt: Fixed in updated patch above
4) Initializing defaultAccessLog may be performed by several threads
- concurrently. That may result in several listener instances being
- added. (It won't break their operation, but it is just a waste).
+ concurrently. That may result in several listener instances being
+ added. (It won't break their operation, but it is just a waste).
+ markt: Agreed it is a waste. Can't do much about it without lots and lots
+ of complexity.
5) in AccessLogListener.containerEvent():
Context context = (Context) event.getData();
- Maybe it'd be better to add an instanceof check before that cast.
+ Maybe it'd be better to add an instanceof check before that cast.
+ markt: Not worth it. That object has to be an instance of Context else all
+ sorts of stuff would break way before the code got this far.
6) In the proposed patch the Listener is only installed iff there
- is an access log there. It should be installed unconditionally.
- E.g., if Tomcat starts without access logs, NoopAccessLog is created.
- As there are no listeners, any change to the configuration
- (e.g. redeploying the ROOT webapp) will pass unnoticed
- and won't reenable the access log.
+ is an access log there. It should be installed unconditionally.
+ E.g., if Tomcat starts without access logs, NoopAccessLog is created.
+ As there are no listeners, any change to the configuration
+ (e.g. redeploying the ROOT webapp) will pass unnoticed
+ and won't reenable the access log.
+ markt: Fixed in updated patch above
* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=48973
Avoid creating file SESSIONS.ser if there's no session.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
