You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by sl...@apache.org on 2010/12/06 21:49:15 UTC
svn commit: r1042786 - in /tomcat/trunk/java/org/apache: catalina/core/
catalina/loader/ tomcat/util/threads/
Author: slaurent
Date: Mon Dec 6 20:49:14 2010
New Revision: 1042786
URL: http://svn.apache.org/viewvc?rev=1042786&view=rev
Log:
bug 49159: Improve ThreadLocal memory leak clean-up
https://issues.apache.org/bugzilla/show_bug.cgi?id=49159
Various fixes after review by markt : formatting, use string manager, svn props...
Modified:
tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties
tomcat/trunk/java/org/apache/catalina/core/StandardContext.java
tomcat/trunk/java/org/apache/catalina/core/StandardThreadExecutor.java
tomcat/trunk/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java (contents, props changed)
tomcat/trunk/java/org/apache/catalina/loader/WebappClassLoader.java
tomcat/trunk/java/org/apache/tomcat/util/threads/Constants.java (contents, props changed)
tomcat/trunk/java/org/apache/tomcat/util/threads/LocalStrings.properties (props changed)
tomcat/trunk/java/org/apache/tomcat/util/threads/TaskQueue.java
tomcat/trunk/java/org/apache/tomcat/util/threads/TaskThread.java (contents, props changed)
tomcat/trunk/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java
Modified: tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties?rev=1042786&r1=1042785&r2=1042786&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties (original)
+++ tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties Mon Dec 6 20:49:14 2010
@@ -238,6 +238,8 @@ standardWrapper.unavailable=Marking serv
standardWrapper.unloadException=Servlet {0} threw unload() exception
standardWrapper.unloading=Cannot allocate servlet {0} because it is being unloaded
standardWrapper.waiting=Waiting for {0} instance(s) to be deallocated
+threadLocalLeakPreventionListener.lifecycleEvent.error=Exception processing lifecycle event {0}
+threadLocalLeakPreventionListener.containerEvent.error=Exception processing container event {0}
defaultInstanceManager.restrictedServletsResource=Restricted servlets property file not found
defaultInstanceManager.privilegedServlet=Servlet of class {0} is privileged and cannot be loaded by this web application
Modified: tomcat/trunk/java/org/apache/catalina/core/StandardContext.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/StandardContext.java?rev=1042786&r1=1042785&r2=1042786&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/StandardContext.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/StandardContext.java Mon Dec 6 20:49:14 2010
@@ -2489,9 +2489,10 @@ public class StandardContext extends Con
return this.renewThreadsWhenStoppingContext;
}
- public void setRenewThreadsWhenStoppingContext(boolean renewThreadsWhenStoppingContext) {
+ public void setRenewThreadsWhenStoppingContext(
+ boolean renewThreadsWhenStoppingContext) {
boolean oldRenewThreadsWhenStoppingContext =
- this.renewThreadsWhenStoppingContext;
+ this.renewThreadsWhenStoppingContext;
this.renewThreadsWhenStoppingContext = renewThreadsWhenStoppingContext;
support.firePropertyChange("renewThreadsWhenStoppingContext",
oldRenewThreadsWhenStoppingContext,
Modified: tomcat/trunk/java/org/apache/catalina/core/StandardThreadExecutor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/StandardThreadExecutor.java?rev=1042786&r1=1042785&r2=1042786&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/StandardThreadExecutor.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/StandardThreadExecutor.java Mon Dec 6 20:49:14 2010
@@ -88,7 +88,8 @@ public class StandardThreadExecutor exte
* renewing all threads at the same time, this delay is observed between 2
* threads being renewed.
*/
- protected long threadRenewalDelay = 1000L;
+ protected long threadRenewalDelay =
+ org.apache.tomcat.util.threads.Constants.DEFAULT_THREAD_RENEWAL_DELAY;
private TaskQueue taskqueue = null;
// ---------------------------------------------- Constructors
Modified: tomcat/trunk/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java?rev=1042786&r1=1042785&r2=1042786&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java Mon Dec 6 20:49:14 2010
@@ -34,25 +34,34 @@ import org.apache.catalina.connector.Con
import org.apache.coyote.ProtocolHandler;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.util.res.StringManager;
import org.apache.tomcat.util.threads.ThreadPoolExecutor;
/**
+ * <p>
* A {@link LifecycleListener} that triggers the renewal of threads in Executor
* pools when a {@link Context} is being stopped to avoid thread-local related
- * memory leaks.<br/>
+ * memory leaks.
+ * </p>
+ * <p>
* Note : active threads will be renewed one by one when they come back to the
* pool after executing their task, see
- * {@link org.apache.tomcat.util.threads.ThreadPoolExecutor}.afterExecute().<br/>
+ * {@link org.apache.tomcat.util.threads.ThreadPoolExecutor}.afterExecute().
+ * </p>
*
* This listener must be declared in server.xml to be active.
*
- * @author slaurent
- *
*/
public class ThreadLocalLeakPreventionListener implements LifecycleListener,
- ContainerListener {
- private static final Log log = LogFactory
- .getLog(ThreadLocalLeakPreventionListener.class);
+ ContainerListener {
+ private static final Log log =
+ LogFactory.getLog(ThreadLocalLeakPreventionListener.class);
+
+ /**
+ * The string manager for this package.
+ */
+ protected static final StringManager sm =
+ StringManager.getManager(Constants.Package);
/**
* Listens for {@link LifecycleEvent} for the start of the {@link Server} to
@@ -63,7 +72,7 @@ public class ThreadLocalLeakPreventionLi
try {
Lifecycle lifecycle = event.getLifecycle();
if (Lifecycle.AFTER_START_EVENT.equals(event.getType())
- && lifecycle instanceof Server) {
+ && lifecycle instanceof Server) {
// when the server starts, we register ourself as listener for
// all context
// as well as container event listener so that we know when new
@@ -73,11 +82,15 @@ public class ThreadLocalLeakPreventionLi
}
if (Lifecycle.AFTER_STOP_EVENT.equals(event.getType())
- && lifecycle instanceof Context) {
+ && lifecycle instanceof Context) {
stopIdleThreads((Context) lifecycle);
}
} catch (Exception e) {
- log.error("Exception processing event " + event, e);
+ String msg =
+ sm.getString(
+ "threadLocalLeakPreventionListener.lifecycleEvent.error",
+ event);
+ log.error(msg, e);
}
}
@@ -87,13 +100,17 @@ public class ThreadLocalLeakPreventionLi
String type = event.getType();
if (Container.ADD_CHILD_EVENT.equals(type)) {
processContainerAddChild(event.getContainer(),
- (Container) event.getData());
+ (Container) event.getData());
} else if (Container.REMOVE_CHILD_EVENT.equals(type)) {
processContainerRemoveChild(event.getContainer(),
- (Container) event.getData());
+ (Container) event.getData());
}
} catch (Exception e) {
- log.error("Exception processing event " + event, e);
+ String msg =
+ sm.getString(
+ "threadLocalLeakPreventionListener.containerEvent.error",
+ event);
+ log.error(msg, e);
}
}
@@ -129,43 +146,35 @@ public class ThreadLocalLeakPreventionLi
protected void processContainerAddChild(Container parent, Container child) {
if (log.isDebugEnabled())
log.debug("Process addChild[parent=" + parent + ",child=" + child
- + "]");
+ + "]");
- try {
- if (child instanceof Context) {
- registerContextListener((Context) child);
- } else if (child instanceof Engine) {
- registerListenersForEngine((Engine) child);
- } else if (child instanceof Host) {
- registerListenersForHost((Host) child);
- }
- } catch (Throwable t) {
- log.error("processContainerAddChild: Throwable", t);
+ if (child instanceof Context) {
+ registerContextListener((Context) child);
+ } else if (child instanceof Engine) {
+ registerListenersForEngine((Engine) child);
+ } else if (child instanceof Host) {
+ registerListenersForHost((Host) child);
}
}
- protected void processContainerRemoveChild(Container parent, Container child) {
+ protected void processContainerRemoveChild(Container parent,
+ Container child) {
if (log.isDebugEnabled())
log.debug("Process removeChild[parent=" + parent + ",child="
- + 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);
- }
- } catch (Throwable t) {
- log.error("processContainerRemoveChild: Throwable", t);
+ 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);
}
-
}
/**
@@ -177,9 +186,8 @@ public class ThreadLocalLeakPreventionLi
* of its parent Service.
*/
private void stopIdleThreads(Context context) {
- if (context instanceof StandardContext
- && !((StandardContext) context)
- .getRenewThreadsWhenStoppingContext()) {
+ if (context instanceof StandardContext &&
+ !((StandardContext) context).getRenewThreadsWhenStoppingContext()) {
log.debug("Not renewing threads when the context is stopping, it is configured not to do it.");
return;
}
@@ -196,10 +204,12 @@ public class ThreadLocalLeakPreventionLi
}
if (executor instanceof ThreadPoolExecutor) {
- ThreadPoolExecutor threadPoolExecutor = (ThreadPoolExecutor) executor;
+ ThreadPoolExecutor threadPoolExecutor =
+ (ThreadPoolExecutor) executor;
threadPoolExecutor.contextStopping();
} else if (executor instanceof StandardThreadExecutor) {
- StandardThreadExecutor stdThreadExecutor = (StandardThreadExecutor) executor;
+ StandardThreadExecutor stdThreadExecutor =
+ (StandardThreadExecutor) executor;
stdThreadExecutor.contextStopping();
}
Propchange: tomcat/trunk/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java
------------------------------------------------------------------------------
svn:eol-style = native
Propchange: tomcat/trunk/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java
------------------------------------------------------------------------------
svn:keywords = Author Date Id Revision
Modified: tomcat/trunk/java/org/apache/catalina/loader/WebappClassLoader.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/loader/WebappClassLoader.java?rev=1042786&r1=1042785&r2=1042786&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/loader/WebappClassLoader.java (original)
+++ tomcat/trunk/java/org/apache/catalina/loader/WebappClassLoader.java Mon Dec 6 20:49:14 2010
@@ -2365,24 +2365,18 @@ public class WebappClassLoader
} catch (IllegalAccessException e) {
log.warn(sm.getString("webappClassLoader.checkThreadLocalsForLeaksFail",
contextName), e);
- } catch (NoSuchMethodException e) {
- log.warn(sm.getString("webappClassLoader.checkThreadLocalsForLeaksFail",
- contextName), e);
- } catch (InvocationTargetException e) {
- log.warn(sm.getString("webappClassLoader.checkThreadLocalsForLeaksFail",
- contextName), e);
}
}
- /*
+ /**
* Analyzes the given thread local map object. Also pass in the field that
* points to the internal table to save re-calculating it on every
* call to this method.
*/
- private void checkThreadLocalMapForLeaks(Object map, Field internalTableField)
- throws NoSuchMethodException, IllegalAccessException,
- NoSuchFieldException, InvocationTargetException {
+ private void checkThreadLocalMapForLeaks(Object map,
+ Field internalTableField) throws IllegalAccessException,
+ NoSuchFieldException {
if (map != null) {
Object[] table = (Object[]) internalTableField.get(map);
if (table != null) {
Modified: tomcat/trunk/java/org/apache/tomcat/util/threads/Constants.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/threads/Constants.java?rev=1042786&r1=1042785&r2=1042786&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/threads/Constants.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/threads/Constants.java Mon Dec 6 20:49:14 2010
@@ -23,4 +23,5 @@ public final class Constants {
public static final String Package = "org.apache.tomcat.util.threads";
+ public static final long DEFAULT_THREAD_RENEWAL_DELAY = 1000L;
}
Propchange: tomcat/trunk/java/org/apache/tomcat/util/threads/Constants.java
------------------------------------------------------------------------------
svn:eol-style = native
Propchange: tomcat/trunk/java/org/apache/tomcat/util/threads/Constants.java
------------------------------------------------------------------------------
svn:keywords = Author Date Id Revision
Propchange: tomcat/trunk/java/org/apache/tomcat/util/threads/LocalStrings.properties
------------------------------------------------------------------------------
svn:eol-style = native
Propchange: tomcat/trunk/java/org/apache/tomcat/util/threads/LocalStrings.properties
------------------------------------------------------------------------------
svn:keywords = Author Date Id Revision
Modified: tomcat/trunk/java/org/apache/tomcat/util/threads/TaskQueue.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/threads/TaskQueue.java?rev=1042786&r1=1042785&r2=1042786&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/threads/TaskQueue.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/threads/TaskQueue.java Mon Dec 6 20:49:14 2010
@@ -81,7 +81,8 @@ public class TaskQueue extends LinkedBlo
@Override
- public Runnable poll(long timeout, TimeUnit unit) throws InterruptedException {
+ public Runnable poll(long timeout, TimeUnit unit)
+ throws InterruptedException {
Runnable runnable = super.poll(timeout, unit);
if (runnable == null && parent != null) {
// the poll timed out, it gives an opportunity to stop the current
@@ -90,21 +91,22 @@ public class TaskQueue extends LinkedBlo
}
return runnable;
}
-
@Override
public Runnable take() throws InterruptedException {
if (parent != null && parent.currentThreadShouldBeStopped()) {
- return poll(parent.getKeepAliveTime(TimeUnit.MILLISECONDS), TimeUnit.MILLISECONDS);
- //yes, this may return null (in case of timeout) which normally does not occur with take()
- //but the ThreadPoolExecutor implementation allows this
+ return poll(parent.getKeepAliveTime(TimeUnit.MILLISECONDS),
+ TimeUnit.MILLISECONDS);
+ // yes, this may return null (in case of timeout) which normally
+ // does not occur with take()
+ // but the ThreadPoolExecutor implementation allows this
}
return super.take();
}
@Override
public int remainingCapacity() {
- if(forcedRemainingCapacity != null) {
+ if (forcedRemainingCapacity != null) {
// ThreadPoolExecutor.setCorePoolSize checks that
// remainingCapacity==0 to allow to interrupt idle threads
// I don't see why, but this hack allows to conform to this
Modified: tomcat/trunk/java/org/apache/tomcat/util/threads/TaskThread.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/threads/TaskThread.java?rev=1042786&r1=1042785&r2=1042786&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/threads/TaskThread.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/threads/TaskThread.java Mon Dec 6 20:49:14 2010
@@ -19,8 +19,6 @@ package org.apache.tomcat.util.threads;
/**
* A Thread implementation that records the time at which it was created.
*
- * @author slaurent
- *
*/
public class TaskThread extends Thread {
Propchange: tomcat/trunk/java/org/apache/tomcat/util/threads/TaskThread.java
------------------------------------------------------------------------------
svn:eol-style = native
Propchange: tomcat/trunk/java/org/apache/tomcat/util/threads/TaskThread.java
------------------------------------------------------------------------------
svn:keywords = Author Date Id Revision
Modified: tomcat/trunk/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java?rev=1042786&r1=1042785&r2=1042786&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java Mon Dec 6 20:49:14 2010
@@ -65,7 +65,7 @@ public class ThreadPoolExecutor extends
/**
* Delay in ms between 2 threads being renewed. If negative, do not renew threads.
*/
- private long threadRenewalDelay = 1000L;
+ private long threadRenewalDelay = Constants.DEFAULT_THREAD_RENEWAL_DELAY;
public ThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, BlockingQueue<Runnable> workQueue, RejectedExecutionHandler handler) {
super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, handler);
@@ -133,9 +133,11 @@ public class ThreadPoolExecutor extends
}
protected boolean currentThreadShouldBeStopped() {
- if (threadRenewalDelay >= 0 && Thread.currentThread() instanceof TaskThread) {
+ if (threadRenewalDelay >= 0
+ && Thread.currentThread() instanceof TaskThread) {
TaskThread currentTaskThread = (TaskThread) Thread.currentThread();
- if (currentTaskThread.getCreationTime() < this.lastContextStoppedTime.longValue()) {
+ if (currentTaskThread.getCreationTime() <
+ this.lastContextStoppedTime.longValue()) {
return true;
}
}
@@ -160,7 +162,8 @@ public class ThreadPoolExecutor extends
* thread, at the discretion of the <tt>Executor</tt> implementation.
* If no threads are available, it will be added to the work queue.
* If the work queue is full, the system will wait for the specified
- * time and it throw a RejectedExecutionException if the queue is still full after that.
+ * time and it throw a RejectedExecutionException if the queue is still
+ * full after that.
*
* @param command the runnable task
* @throws RejectedExecutionException if this task cannot be
@@ -197,7 +200,8 @@ public class ThreadPoolExecutor extends
// save the current pool parameters to restore them later
int savedCorePoolSize = this.getCorePoolSize();
- TaskQueue taskQueue = getQueue() instanceof TaskQueue ? (TaskQueue) getQueue() : null;
+ TaskQueue taskQueue =
+ getQueue() instanceof TaskQueue ? (TaskQueue) getQueue() : null;
if (taskQueue != null) {
// note by slaurent : quite oddly threadPoolExecutor.setCorePoolSize
// checks that queue.remainingCapacity()==0. I did not understand
@@ -210,16 +214,18 @@ public class ThreadPoolExecutor extends
this.setCorePoolSize(0);
// wait a little so that idle threads wake and poll the queue again,
- // this time always with a timeout (queue.poll() instead of queue.take())
- // even if we did not wait enough, TaskQueue.take() takes care of timing out
- // so that we are sure that all threads of the pool are renewed in a limited
- // time, something like (threadKeepAlive + longest request time)
+ // this time always with a timeout (queue.poll() instead of
+ // queue.take())
+ // even if we did not wait enough, TaskQueue.take() takes care of timing
+ // out, so that we are sure that all threads of the pool are renewed in
+ // a limited time, something like
+ // (threadKeepAlive + longest request time)
try {
Thread.sleep(200L);
} catch (InterruptedException e) {
- //yes, ignore
+ // yes, ignore
}
-
+
if (taskQueue != null) {
// ok, restore the state of the queue and pool
taskQueue.setForcedRemainingCapacity(null);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: svn commit: r1042786 - in /tomcat/trunk/java/org/apache:
catalina/core/ catalina/loader/ tomcat/util/threads/
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: Mark Thomas [mailto:markt@apache.org]
> Subject: Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache:
> catalina/core/ catalina/loader/ tomcat/util/threads/
> My preference is for operator before since I believe it aids
> readability. YMMV.
Strongly agree. I've been programming in a multitude of languages for over forty years, and I find operator before line break to be a much more readable (and aesthetically pleasing) arrangement.
Sun's Java programming conventions haven't been updated in over a decade, and were written before many people had much experience with the language. It's unfortunate they haven't been kept up to date with newer productivity findings. (I also find it quite curious that they specify break after comma, but before other operators - we don't need no stinkin' consistency.)
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache:
catalina/core/ catalina/loader/ tomcat/util/threads/
Posted by Konstantin Kolinko <kn...@gmail.com>.
2010/12/8 Sylvain Laurent <sy...@m4x.org>:
>
> On 7 déc. 2010, at 17:35, Mark Thomas wrote:
>> +1. Line length is an area I am becoming increasingly happy to be
>> flexible. Aim for 80 but if 81 or 82 (or anything < ~90) makes the code
>> more readable then I'd be fine with it.
>
> As a rookie, I did not want to start bothering you with this, but since you bring the topic...
> I think 80 is somewhat outdated: screens are now bigger, and especially wider (16:9 vs 4:3).
> I think we should allow a greater line length to reduce the code height (the number of lines).
> My preference is 100 characters per line.
IDEs are fine, but long lines are hard to deal with
1) in commit e-mails - they become wrapped
2) in side-by-side diffs
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache: catalina/core/
catalina/loader/ tomcat/util/threads/
Posted by Mark Thomas <ma...@apache.org>.
On 07/12/2010 22:22, Sylvain Laurent wrote:
>
> On 7 déc. 2010, at 17:35, Mark Thomas wrote:
>> +1. Line length is an area I am becoming increasingly happy to be
>> flexible. Aim for 80 but if 81 or 82 (or anything < ~90) makes the code
>> more readable then I'd be fine with it.
>
> As a rookie, I did not want to start bothering you with this, but since you bring the topic...
We are both committers and so your opinion holds as much weight as mine
or any other committer.
> I think 80 is somewhat outdated: screens are now bigger, and especially wider (16:9 vs 4:3).
> I think we should allow a greater line length to reduce the code height (the number of lines).
> My preference is 100 characters per line.
That is a fair point. The times I work on a screen where >80 is an issue
are far, far fewer than the times when a longer line length would make
the code more readable.
The way I like to have my IDE set up, 100 is a little too wide. 90 is
manageable which is why I said usually 80 with the flexibility to go to
90 if it makes the code more readable. Of course, I have been looking
for an excuse to buy myself a nice new wide-screen monitor ;)
Based on a couple of classes I have been working on recently, a longer
line length doesn't seem to offer too much in the way of fewer lines. My
guess is 80->90 reduces line count by ~3% and 90->100 by a similar
amount. I am sure it will vary by class.
My own view is that we should generally stick to 80 but treat it as a
guideline and use longer lines where it improves readability but lines
longer than 90 really should be the exception when there is no place
available to break the line.
As I said, all this is just my opinion. There is no formal limit but the
current code generally sticks to 80.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache: catalina/core/ catalina/loader/ tomcat/util/threads/
Posted by Sylvain Laurent <sy...@m4x.org>.
On 7 déc. 2010, at 17:35, Mark Thomas wrote:
> +1. Line length is an area I am becoming increasingly happy to be
> flexible. Aim for 80 but if 81 or 82 (or anything < ~90) makes the code
> more readable then I'd be fine with it.
As a rookie, I did not want to start bothering you with this, but since you bring the topic...
I think 80 is somewhat outdated: screens are now bigger, and especially wider (16:9 vs 4:3).
I think we should allow a greater line length to reduce the code height (the number of lines).
My preference is 100 characters per line.
Sylvain
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache: catalina/core/
catalina/loader/ tomcat/util/threads/
Posted by Mark Thomas <ma...@apache.org>.
On 07/12/2010 14:48, Konstantin Kolinko wrote:
> 2010/12/7 Mark Thomas <ma...@apache.org>:
>>
>> My preference is for operator before since I believe it aids readability.
>> YMMV.
>>
>>> So is operator after line wrapping really a tomcat standard?
>>
>> operator before line wrapping is the direction the code is heading in and
>> the direction I think it should continue to head.
>>
>
> My personal preference is that the new code formatting were more or
> less consistent with the rest of the file.
+1
> Trying to enforce a style here goes nowhere: such changes cannot be
> backported to TC6, and you cannot limit checkstyle to check only files
> created after certain date.
Before any checkstyle check can be enabled, the whole code-base has to
be changed to conform to the check. This tends to be something I do when
I have spare 30 mins I can't do anything else in, I'm stuck somewhere
without internet connectivity or I just feel the need to do something
fairly mindless for a while.
I usually add the checks I am thinking of working on to the checksyle
file in advance of doing the work - commented out with an indication of
the current number of failures - to give folks a chance to comment.
> If you have some "long string" that occupies all space up to position
> 79, forcing some operator after it to stay on the same line
> effectively increases its length by several chars and will cause the
> whole string to be placed on the new line. It just wastes space and
> becomes ugly.
+1. Line length is an area I am becoming increasingly happy to be
flexible. Aim for 80 but if 81 or 82 (or anything < ~90) makes the code
more readable then I'd be fine with it.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: svn commit: r1042786 - in /tomcat/trunk/java/org/apache:
catalina/core/ catalina/loader/ tomcat/util/threads/
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: Konstantin Kolinko [mailto:knst.kolinko@gmail.com]
> Subject: Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache:
> catalina/core/ catalina/loader/ tomcat/util/threads/
> If you have some "long string" that occupies all space up to position
> 79, forcing some operator after it to stay on the same line
> effectively increases its length by several chars and will cause the
> whole string to be placed on the new line. It just wastes space and
> becomes ugly.
Agreed, which is why Mark's suggestion of making those static final String fields is even more compelling.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache:
catalina/core/ catalina/loader/ tomcat/util/threads/
Posted by Konstantin Kolinko <kn...@gmail.com>.
2010/12/7 Mark Thomas <ma...@apache.org>:
>
> My preference is for operator before since I believe it aids readability.
> YMMV.
>
>> So is operator after line wrapping really a tomcat standard?
>
> operator before line wrapping is the direction the code is heading in and
> the direction I think it should continue to head.
>
My personal preference is that the new code formatting were more or
less consistent with the rest of the file. I *do not* care where
operator is on that line.
If it is readable, it is fine with me. I see no value in bothering
about it besides some aesthetics.
(It makes no difference in Java. Though in Javascript placing operator
at the end of the line is preferred, because of implicit ';' that can
otherwise split the line in two).
Trying to enforce a style here goes nowhere: such changes cannot be
backported to TC6, and you cannot limit checkstyle to check only files
created after certain date.
2010/12/7 Caldarale, Charles R <Ch...@unisys.com>:
> Strongly agree. I've been programming in a multitude of languages for over forty years, and I find operator before line break to be a much more readable (and aesthetically pleasing) arrangement.
>
If you have some "long string" that occupies all space up to position
79, forcing some operator after it to stay on the same line
effectively increases its length by several chars and will cause the
whole string to be placed on the new line. It just wastes space and
becomes ugly.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache:
catalina/core/ catalina/loader/ tomcat/util/threads/
Posted by Felix Schumacher <fe...@internetallee.de>.
Am Dienstag, den 07.12.2010, 13:28 +0000 schrieb Mark Thomas:
> On 07/12/2010 12:29, Felix Schumacher wrote:
> > Am Montag, den 06.12.2010, 21:22 +0000 schrieb Mark Thomas:
> >>> - log.error("Exception processing event " + event, e);
> >>> + String msg =
> >>> + sm.getString(
> >>> + "threadLocalLeakPreventionListener.lifecycleEvent.error",
> >>> + event);
> >>> + log.error(msg, e);
> >>
> >> More auto formatting "helpfulness" by the look of it. No reason for
> >> String msg = sm.getString(
> >> not to be on one line.
> > It would probably break the 80 character limits :)
>
> I don't get this. I was suggesting the following:
> String msg = sm.getString(
> "threadLocalLeakPreventionListener.lifecycleEvent.error",
> event);
> i.e. get rid of the unnecessary line break.
Oh, didn't think of that one, you are right :)
>
> >> If we can pull some Eclipse settings together to help folks with this
> >> that would be great.
> > That would be great, if we knew what the conventions were.
> > http://tomcat.apache.org/getinvolved.html only mentions four
> > conventions.
>
> I'm not sure on the best way to approach this. The code is not
> consistent. The committers don't have a consistent style for new stuff.
> I'm not sure how much we can agree on.
>
> What I have been doing is slowly adding rules to Checkstyle. So far
> there hasn't been any push back but I'm sure there will be some at some
> point. My idea was to add the rules the community was happy with and
> where we disagree leave things as they are. Modifier order and redundant
> modifiers are the next ones on the list. I'll see if there is an
> operator placement one ... there is so I'll add that as well (commented
> out until the issues are fixed).
Great, I really love to use eclipse auto formatting, but that can only
be used, if we commit ourselves onto one style.
If we have already commited the style by checkstyle rules, we should
prepare an eclipse formatting rule that matches those.
Regards
Felix
>
> Mark
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache: catalina/core/
catalina/loader/ tomcat/util/threads/
Posted by Mark Thomas <ma...@apache.org>.
On 07/12/2010 13:28, Mark Thomas wrote:
> On 07/12/2010 12:29, Felix Schumacher wrote:
>> Am Montag, den 06.12.2010, 21:22 +0000 schrieb Mark Thomas:
>>> That indenting is now misleading (it was fine before) and the operator
>>> is still at the beginning of the line.
>> I just made a quick lookup for usage of (&&) operator at the end of line
>> versus (&&) operator at the beginning of the next line. It seems that
>> usage has changed between tomcat 6 and trunk.
>
> For the stats to be meaningful you'd need to look at all the possible
> operators, not just &&. That said, I suspect you'll see a similar thing,
> a gradual shift towards operator before line wrapping
Based on the Checkstyle error count for Tomcat 7:
operator before new line 2022
operator after new line 1077
So there is a preference for operator before but there are plenty of
counter examples.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache: catalina/core/
catalina/loader/ tomcat/util/threads/
Posted by Mark Thomas <ma...@apache.org>.
On 07/12/2010 12:29, Felix Schumacher wrote:
> Am Montag, den 06.12.2010, 21:22 +0000 schrieb Mark Thomas:
>> That indenting is now misleading (it was fine before) and the operator
>> is still at the beginning of the line.
> I just made a quick lookup for usage of (&&) operator at the end of line
> versus (&&) operator at the beginning of the next line. It seems that
> usage has changed between tomcat 6 and trunk.
For the stats to be meaningful you'd need to look at all the possible
operators, not just &&. That said, I suspect you'll see a similar thing,
a gradual shift towards operator before line wrapping
> Java coding conventions and eclipse coding conventions both prefer
> operator after line wrapping.
My preference is for operator before since I believe it aids
readability. YMMV.
> So is operator after line wrapping really a tomcat standard?
operator before line wrapping is the direction the code is heading in
and the direction I think it should continue to head.
>>> - log.error("Exception processing event " + event, e);
>>> + String msg =
>>> + sm.getString(
>>> + "threadLocalLeakPreventionListener.lifecycleEvent.error",
>>> + event);
>>> + log.error(msg, e);
>>
>> More auto formatting "helpfulness" by the look of it. No reason for
>> String msg = sm.getString(
>> not to be on one line.
> It would probably break the 80 character limits :)
I don't get this. I was suggesting the following:
String msg = sm.getString(
"threadLocalLeakPreventionListener.lifecycleEvent.error",
event);
i.e. get rid of the unnecessary line break.
>> If we can pull some Eclipse settings together to help folks with this
>> that would be great.
> That would be great, if we knew what the conventions were.
> http://tomcat.apache.org/getinvolved.html only mentions four
> conventions.
I'm not sure on the best way to approach this. The code is not
consistent. The committers don't have a consistent style for new stuff.
I'm not sure how much we can agree on.
What I have been doing is slowly adding rules to Checkstyle. So far
there hasn't been any push back but I'm sure there will be some at some
point. My idea was to add the rules the community was happy with and
where we disagree leave things as they are. Modifier order and redundant
modifiers are the next ones on the list. I'll see if there is an
operator placement one ... there is so I'll add that as well (commented
out until the issues are fixed).
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache:
catalina/core/ catalina/loader/ tomcat/util/threads/
Posted by Felix Schumacher <fe...@internetallee.de>.
Am Montag, den 06.12.2010, 21:22 +0000 schrieb Mark Thomas:
> On 06/12/2010 20:49, slaurent@apache.org wrote:
> > Author: slaurent
> > Date: Mon Dec 6 20:49:14 2010
> > New Revision: 1042786
> >
> > URL: http://svn.apache.org/viewvc?rev=1042786&view=rev
>
>
> > @@ -63,7 +72,7 @@ public class ThreadLocalLeakPreventionLi
> > try {
> > Lifecycle lifecycle = event.getLifecycle();
> > if (Lifecycle.AFTER_START_EVENT.equals(event.getType())
> > - && lifecycle instanceof Server) {
> > + && lifecycle instanceof Server) {
> > // when the server starts, we register ourself as listener for
> > // all context
> > // as well as container event listener so that we know when new
>
> That indenting is now misleading (it was fine before) and the operator
> is still at the beginning of the line.
I just made a quick lookup for usage of (&&) operator at the end of line
versus (&&) operator at the beginning of the next line. It seems that
usage has changed between tomcat 6 and trunk.
In tomcat6.x it is
> egrep -r "&&\s*$" * | wc -l # && at the end of line
217
> egrep -r "^\s*&&" * | wc -l # && at front of line
228
that is slightly in favor of operator after line wrapping.
In tomcat.trunk it is now
> egrep -r "&&\s*$" * | wc -l # && at the end of line
279
> egrep -r "^\s*&&" * | wc -l # && at front of line
264
Which is favors operator before line wrapping.
Java coding conventions and eclipse coding conventions both prefer
operator after line wrapping.
So is operator after line wrapping really a tomcat standard?
>
>
> > - log.error("Exception processing event " + event, e);
> > + String msg =
> > + sm.getString(
> > + "threadLocalLeakPreventionListener.lifecycleEvent.error",
> > + event);
> > + log.error(msg, e);
>
> More auto formatting "helpfulness" by the look of it. No reason for
> String msg = sm.getString(
> not to be on one line.
It would probably break the 80 character limits :)
>
> If we can pull some Eclipse settings together to help folks with this
> that would be great.
That would be great, if we knew what the conventions were.
http://tomcat.apache.org/getinvolved.html only mentions four
conventions.
>
> Mark
Bye
Felix
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1042786 - in /tomcat/trunk/java/org/apache: catalina/core/
catalina/loader/ tomcat/util/threads/
Posted by Mark Thomas <ma...@apache.org>.
On 06/12/2010 20:49, slaurent@apache.org wrote:
> Author: slaurent
> Date: Mon Dec 6 20:49:14 2010
> New Revision: 1042786
>
> URL: http://svn.apache.org/viewvc?rev=1042786&view=rev
> @@ -63,7 +72,7 @@ public class ThreadLocalLeakPreventionLi
> try {
> Lifecycle lifecycle = event.getLifecycle();
> if (Lifecycle.AFTER_START_EVENT.equals(event.getType())
> - && lifecycle instanceof Server) {
> + && lifecycle instanceof Server) {
> // when the server starts, we register ourself as listener for
> // all context
> // as well as container event listener so that we know when new
That indenting is now misleading (it was fine before) and the operator
is still at the beginning of the line.
> - log.error("Exception processing event " + event, e);
> + String msg =
> + sm.getString(
> + "threadLocalLeakPreventionListener.lifecycleEvent.error",
> + event);
> + log.error(msg, e);
More auto formatting "helpfulness" by the look of it. No reason for
String msg = sm.getString(
not to be on one line.
If we can pull some Eclipse settings together to help folks with this
that would be great.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org