Hi, 2017-06-27 22:59 GMT+03:00 Rainer Jung <rainer.j...@kippdata.de>: > > Am 27.06.2017 um 21:41 schrieb Rainer Jung: >> >> Hi Violeta, >> >> Am 16.06.2017 um 21:17 schrieb violet...@apache.org: >>> >>> Author: violetagg >>> Date: Fri Jun 16 19:17:39 2017 >>> New Revision: 1798977 >>> >>> URL: http://svn.apache.org/viewvc?rev=1798977&view=rev >>> Log: >>> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=61105 >>> Add a new JULI FileHandler configuration for specifying the maximum >>> number of days to keep the log files. By default the log files will be >>> kept 90 days as configured in logging.properties. >>> >>> Added: >>> tomcat/trunk/test/org/apache/juli/TestFileHandler.java (with props) >>> Modified: >>> tomcat/trunk/conf/logging.properties >>> tomcat/trunk/java/org/apache/juli/AsyncFileHandler.java >>> tomcat/trunk/java/org/apache/juli/FileHandler.java >>> tomcat/trunk/webapps/docs/changelog.xml >>> tomcat/trunk/webapps/docs/logging.xml >> >> >> ... >> >>> Modified: tomcat/trunk/java/org/apache/juli/FileHandler.java >>> URL: >>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/juli/FileHandler.java?rev=1798977&r1=1798976&r2=1798977&view=diff >>> >>> ============================================================================== >>> >>> --- tomcat/trunk/java/org/apache/juli/FileHandler.java (original) >>> +++ tomcat/trunk/java/org/apache/juli/FileHandler.java Fri Jun 16 >>> 19:17:39 2017 >> >> ... >> >>> @@ -74,24 +84,37 @@ import java.util.logging.LogRecord; >>> * <li><code>formatter</code> - The >>> <code>java.util.logging.Formatter</code> >>> * implementation class name for this Handler. Default value: >>> * <code>java.util.logging.SimpleFormatter</code></li> >>> + * <li><code>maxDays</code> - The maximum number of days to keep >>> the log >>> + * files. If the specified value is <code><=0</code> then the >>> log files >>> + * will be kept on the file system forever, otherwise they will be >>> kept the >>> + * specified maximum days. Default value: <code>-1</code>.</li> >>> * </ul> >>> */ >>> public class FileHandler extends Handler { >>> + public static final int DEFAULT_MAX_DAYS = -1; >>> + >>> + private static final ExecutorService DELETE_FILES_SERVICE = >>> Executors.newSingleThreadExecutor(); >> >> >> When testing the M22 release I noticed that a tomcat process was >> leftover that didn't want to shut down. I checked and I could easily >> reproduce by starting with startup.sh and then stopping with shutdown.sh >> (but not using kill). IMHO it didn't shut down, because according to a >> thread dump it had a single non-daemon thread still running (named >> "pool-1-thread-1"). Since we typically give more specific names to all >> threads we create I instrumented the Thread class to find out the >> creator of that thread and noticed, that it was created here: >> >> at java.lang.Thread.<init>(Thread.java:677) >> at >> java.util.concurrent.Executors$DefaultThreadFactory.newThread(Executors.java:613) >> >> at >> java.util.concurrent.ThreadPoolExecutor$Worker.<init>(ThreadPoolExecutor.java:612) >> >> at >> java.util.concurrent.ThreadPoolExecutor.addWorker(ThreadPoolExecutor.java:925) >> >> at >> java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1357) >> >> at >> java.util.concurrent.AbstractExecutorService.submit(AbstractExecutorService.java:112) >> >> at >> java.util.concurrent.Executors$DelegatedExecutorService.submit(Executors.java:678) >> >> at org.apache.juli.FileHandler.clean(FileHandler.java:463) >> at org.apache.juli.FileHandler.<init>(FileHandler.java:117) >> at >> org.apache.juli.AsyncFileHandler.<init>(AsyncFileHandler.java:82) >> at >> org.apache.juli.AsyncFileHandler.<init>(AsyncFileHandler.java:74) >> >> So it is the thread belonging to the above "ExecutorService >> DELETE_FILES_SERVICE". I could not see, how that thread would ever get >> stopped, so two remarks: >> >> - in order to make sure TC can shut down without problem we either need >> to stop that thread by ourselves during TC shutdown or mark it as a >> daemon thread. I guess the latter is preferred. >> >> - we should give the created thread a specific name according to its >> typical task so that it can be identified in any thread dump. That >> should be doable by the same ThreadFactory. >> >> So we need to pass a ThreadFactory to the newSingleThreadExecutor() call >> (this possibility already exists in the Executors class). To keep juli >> independent from the other TC classes, we probably need to code the >> factory inside juli, but we can borrow code from the small class >> org.apache.tomcat.util.threads.TaskThreadFactory or use code from there >> to extend the result of Executors.defaultThreadFactory() or >> Executors.privilegedThreadFactory(). > > > I should add: I only observed it for TC 9, because there the feature is active by default due to the updated juli config file, so the submit to the executor happens (attribute maxDays in logging.properties). > > On all other branches the feature is off by default and although the executor gets created, I do not see the thread which probably means it gets created lazily during the first submit. That submit never happens with the default config, but would once someone actually tries to use the feature. > > So of course we should fix on all branches and the feature is somewhat broken on all branches currently, not only on TC 9. >
Thanks for the review. I committed a change that addresses the issues. In addition I spotted that we need additional permission in order to be able to delete file when running under Security Manager. Regards, Violeta > > Regards, > > Rainer > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org >
