You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2022/08/20 14:06:25 UTC
[GitHub] [tomcat] ppkarwasz opened a new pull request, #545: Replace `LoggerThread` with single-threaded executor
ppkarwasz opened a new pull request, #545:
URL: https://github.com/apache/tomcat/pull/545
This provides an alternative solution to the `LoggerThread` problems described in #544.
It replaces `LoggerThread` with a single-threaded `ExecutorService` almost identical to the one used by `FileHandler`. This improves `AsyncFileHandler` in two aspects:
* the thread created by the executor service always uses Tomcat JULI's classloader as TCCL, preventing a memory leak,
* if an `Error` kills the working thread, it is regenerated by the executor service.
A test is provided to ensure that the previous overflow policies work as documented.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] aooohan commented on a diff in pull request #545: Replace `LoggerThread` with single-threaded executor
Posted by GitBox <gi...@apache.org>.
aooohan commented on code in PR #545:
URL: https://github.com/apache/tomcat/pull/545#discussion_r950959440
##########
java/org/apache/juli/AsyncFileHandler.java:
##########
@@ -54,17 +60,12 @@ public class AsyncFileHandler extends FileHandler {
System.getProperty("org.apache.juli.AsyncMaxRecordCount",
Integer.toString(DEFAULT_MAX_RECORDS)));
- protected static final LinkedBlockingDeque<LogEntry> queue =
- new LinkedBlockingDeque<>(MAX_RECORDS);
-
- protected static final LoggerThread logger = new LoggerThread();
-
- static {
- logger.start();
- }
+ private static final ExecutorService LOGGER_SERVICE = new LoggerExecutorService(OVERFLOW_DROP_TYPE,
+ DEFAULT_MAX_RECORDS);
Review Comment:
There should use `MAX_RECORDS`,not `DEFAULT_MAX_RECORDS`.
##########
java/org/apache/juli/AsyncFileHandler.java:
##########
@@ -117,115 +120,82 @@ public void publish(LogRecord record) {
// fill source entries, before we hand the record over to another
// thread with another class loader
record.getSourceMethodName();
- LogEntry entry = new LogEntry(record, this);
- boolean added = false;
- try {
- while (!added && !queue.offer(entry)) {
- switch (OVERFLOW_DROP_TYPE) {
- case OVERFLOW_DROP_LAST: {
- //remove the last added element
- queue.pollLast();
- break;
- }
- case OVERFLOW_DROP_FIRST: {
- //remove the first element in the queue
- queue.pollFirst();
- break;
- }
- case OVERFLOW_DROP_FLUSH: {
- added = queue.offer(entry, 1000, TimeUnit.MILLISECONDS);
- break;
- }
- case OVERFLOW_DROP_CURRENT: {
- added = true;
- break;
- }
- }//switch
- }//while
- } catch (InterruptedException x) {
- // Allow thread to be interrupted and back out of the publish
- // operation. No further action required.
- }
+ loggerService.execute(new Runnable() {
+ @Override
+ public void run() {
+ if (!closed) {
+ publishInternal(record);
+ }
+ }
+ });
}
protected void publishInternal(LogRecord record) {
super.publish(record);
}
- protected static class LoggerThread extends Thread {
-
- /*
- * Implementation note: Use of this count could be extended to
- * start/stop the LoggerThread but that would require careful locking as
- * the current size of the queue also needs to be taken into account and
- * there are lost of edge cases when rapidly starting and stopping
- * handlers.
- */
- private static final AtomicInteger handlerCount = new AtomicInteger();
-
- public static void registerHandler() {
- handlerCount.incrementAndGet();
+ static class LoggerExecutorService extends ThreadPoolExecutor {
+
+ private static final ThreadFactory THREAD_FACTORY = new ThreadFactory(THREAD_PREFIX);
+
+ public LoggerExecutorService(final int overflowDropType, final int maxRecords) {
+ super(1, 1, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingDeque<>(maxRecords), THREAD_FACTORY);
+ switch (overflowDropType) {
+ case OVERFLOW_DROP_LAST:
+ default:
+ setRejectedExecutionHandler(new DropLastPolicy());
+ break;
+ case OVERFLOW_DROP_FIRST:
+ setRejectedExecutionHandler(new DiscardOldestPolicy());
+ break;
+ case OVERFLOW_DROP_FLUSH:
+ setRejectedExecutionHandler(new DropFlushPolicy());
+ break;
+ case OVERFLOW_DROP_CURRENT:
+ setRejectedExecutionHandler(new DiscardPolicy());
+ }
}
- public static void deregisterHandler() {
- int newCount = handlerCount.decrementAndGet();
- if (newCount == 0) {
- try {
- Thread dummyHook = new Thread();
- Runtime.getRuntime().addShutdownHook(dummyHook);
- Runtime.getRuntime().removeShutdownHook(dummyHook);
- } catch (IllegalStateException ise) {
- // JVM is shutting down.
- // Allow up to 10s for for the queue to be emptied
- int sleepCount = 0;
- while (!AsyncFileHandler.queue.isEmpty() && sleepCount < 10000) {
- try {
- Thread.sleep(1);
- } catch (InterruptedException e) {
- // Ignore
- }
- sleepCount++;
- }
- }
- }
+ @Override
+ public LinkedBlockingDeque<Runnable> getQueue() {
+ return (LinkedBlockingDeque<Runnable>) super.getQueue();
}
- public LoggerThread() {
- this.setDaemon(true);
- this.setName("AsyncFileHandlerWriter-" + System.identityHashCode(this));
+ @Override
+ public void finalize() {
+ shutdown();
}
+ }
+
+ private static class DropFlushPolicy implements RejectedExecutionHandler {
@Override
- public void run() {
+ public void rejectedExecution(Runnable r, ThreadPoolExecutor executor) {
while (true) {
+ if (executor.isShutdown()) {
+ break;
+ }
try {
- LogEntry entry = queue.take();
- entry.flush();
- } catch (InterruptedException x) {
- // Ignore the attempt to interrupt the thread.
- } catch (Exception x) {
- x.printStackTrace();
+ if (executor.getQueue().offer(r, 1000, TimeUnit.MILLISECONDS)) {
+ break;
+ }
+ ;
Review Comment:
This line needs to be removed.
##########
test/org/apache/juli/TestAsyncFileHandlerOverflow.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.juli;
+
+import static org.junit.Assert.assertEquals;
Review Comment:
Please do not use static import for the assertion tool as it is not very clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf closed pull request #545: Replace `LoggerThread` with single-threaded executor
Posted by GitBox <gi...@apache.org>.
markt-asf closed pull request #545: Replace `LoggerThread` with single-threaded executor
URL: https://github.com/apache/tomcat/pull/545
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on pull request #545: Replace `LoggerThread` with single-threaded executor
Posted by GitBox <gi...@apache.org>.
markt-asf commented on PR #545:
URL: https://github.com/apache/tomcat/pull/545#issuecomment-1226902105
I think this is a better approach than the current code that uses a Thread. Therefore, I think we can close #544.
I have been reviewing the PR and overall I like it. I have noticed that it is possible to lose log messages during shutdown (add a 100ms delay just before the call to publish internal to simulate a large backlog). The threading approach originally suffered from this problem as well and I think the same solution will work (register uses of the AsyncHandler and explicitly shutdown the executor during JULI shutdown). I plan on working on this today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on pull request #545: Replace `LoggerThread` with single-threaded executor
Posted by GitBox <gi...@apache.org>.
markt-asf commented on PR #545:
URL: https://github.com/apache/tomcat/pull/545#issuecomment-1227323763
Applied manually so I could address the lost messages on shutdown issue.
Thanks for the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ppkarwasz commented on pull request #545: Replace `LoggerThread` with single-threaded executor
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #545:
URL: https://github.com/apache/tomcat/pull/545#issuecomment-1227607499
@markt-asf, thanks for taking care of this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ppkarwasz commented on a diff in pull request #545: Replace `LoggerThread` with single-threaded executor
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #545:
URL: https://github.com/apache/tomcat/pull/545#discussion_r950998700
##########
java/org/apache/juli/AsyncFileHandler.java:
##########
@@ -54,17 +60,12 @@ public class AsyncFileHandler extends FileHandler {
System.getProperty("org.apache.juli.AsyncMaxRecordCount",
Integer.toString(DEFAULT_MAX_RECORDS)));
- protected static final LinkedBlockingDeque<LogEntry> queue =
- new LinkedBlockingDeque<>(MAX_RECORDS);
-
- protected static final LoggerThread logger = new LoggerThread();
-
- static {
- logger.start();
- }
+ private static final ExecutorService LOGGER_SERVICE = new LoggerExecutorService(OVERFLOW_DROP_TYPE,
+ DEFAULT_MAX_RECORDS);
Review Comment:
Corrected, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org