Author: markt
Date: Tue Jan 4 22:53:25 2011
New Revision: 1055232
URL: http://svn.apache.org/viewvc?rev=1055232&view=rev
Log:
Vote
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=1055232&r1=1055231&r2=1055232&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Tue Jan 4 22:53:25 2011
@@ -204,59 +204,8 @@ PATCHES PROPOSED TO BACKPORT:
* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=50201
Handle ROOT webapp redeployment, host start/stop etc for default access log
- 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/
- 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.
- 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?).
- 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).
- 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.
- 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.
- markt: Fixed in updated patch above
-
- kkolinko: (Re: markt's 2010-12-05-bug50201-tc6.patch)
- 7) Why "Lifecycle.DESTROY_EVENT.endsWith(type)" in a condition?
- s/endsWith/equals/
- markt: Auto-complete snafu - fixed in trunk
- 8) I do not see how the listener can be reused when recalculation
- happens.
- (We are now installing a new listener when NoAccessLog wins,
- so my understanding is that it is no more necessary to wait for
- reuse there. The new listener will catch all the necessary events).
- Thus all events that force recalculation should result
- in disabling the listener.
- 9) I propose to use an AtomicReference to guard against registering
- duplicate listeners (a fix for 4)). See the patch.
- 10) I added install() and uninstall() method to AccessLogListener
- to help register/unregister the listeners.
- 8),9),10) were applied to trunk in r1045003
http://people.apache.org/~kkolinko/patches/2010-12-07_tc6_bug50201.patch
- +1: kkolinko
+ +1: kkolinko, markt
-1:
* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=48973
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
