On 05/12/2010 22:54, slaur...@apache.org wrote:
> Author: slaurent
> Date: Sun Dec 5 22:54:05 2010
> New Revision: 1042482
>
> URL: http://svn.apache.org/viewvc?rev=1042482&view=rev
General comments:
Numerous new lines with length > 80 chars.
ASF policy is to strongly discourage author tags.
Your svn client should be configured to set svn:eol-style native on new
text files.
I did wonder about the fixing over time nature of this approach vs. the
immediate fixing of the previous approach and whether or not we should
give users the option of both. On reflection I think providing both will
add a fair amount of complexity and given that this is trying to fix
what is essentially an application bug then I think just having one
approach is fine and this is a better approach than the previous
implementation.
Specific comments in-line.
> Modified:
> tomcat/trunk/ (props changed)
> tomcat/trunk/conf/ (props changed)
> tomcat/trunk/webapps/ (props changed)
-1 to all these changes.
a) This is not a Tomcat instance for running. That is created in output.
b) It reverts a useful change I made earlier today
> Added:
> tomcat/trunk/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java
> + private static final Log log = LogFactory
> + .getLog(ThreadLocalLeakPreventionListener.class);
That is an odd place for a line-break that I find hard to read. I find
either of the following easier to read:
private static final Log log =
LogFactory.getLog(ThreadLocalLeakPreventionListener.class);
private static final Log log = LogFactory.getLog(
ThreadLocalLeakPreventionListener.class);
There are multiple instances of this throughout the commit.
> + public void lifecycleEvent(LifecycleEvent event) {
> + try {
> + Lifecycle lifecycle = event.getLifecycle();
> + if (Lifecycle.AFTER_START_EVENT.equals(event.getType())
> + && lifecycle instanceof Server) {
With the operator on the new line it is easy to miss what is going on.
Generally, Tomcat style is to put the operator on the first line. e.g.
if (Lifecycle.AFTER_START_EVENT.equals(event.getType()) &&
lifecycle instanceof Server) {
There are multiple instances of this throughout the commit.
> + } catch (Exception e) {
> + log.error("Exception processing event " + event, e);
> + }
Error messages really should be using a StringManager.
There are multiple instances of this throughout the commit.
> + protected void processContainerRemoveChild(Container parent, Container
> child) {
> +
> + if (log.isDebugEnabled())
> + log.debug("Process removeChild[parent=" + parent + ",child="
> + + child + "]");
> +
> + try {
> + if (child instanceof Context) {
> + Context context = (Context) child;
> + context.removeLifecycleListener(this);
> + } else if (child instanceof Host) {
> + Host host = (Host) child;
> + host.removeContainerListener(this);
> + } else if (child instanceof Engine) {
> + Engine engine = (Engine) child;
> + engine.removeContainerListener(this);
> + }
Why all this? Just use:
child.removeLifecycleListener(this);
Does the fact the Container implements Lifecycle simplify any of the
other code here?
> Modified: tomcat/trunk/java/org/apache/catalina/loader/WebappClassLoader.java
> - private void clearThreadLocalMap(Object map, Field internalTableField)
> + private void checkThreadLocalMapForLeaks(Object map, Field
> internalTableField)
> throws NoSuchMethodException, IllegalAccessException,
> NoSuchFieldException, InvocationTargetException {
Not all of those Exceptions are required after this commit.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
