[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api

["ppkarwasz (via GitHub)" <gi...@apache.org>]

ppkarwasz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1094968023


##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java:
##########
@@ -2742,11 +2739,8 @@ public LogBuilder atFatal() {
      */
     @Override
     public LogBuilder always() {
-        final DefaultLogBuilder builder = logBuilder.get();
-        if (builder.isInUse()) {
-            return new DefaultLogBuilder(this);
-        }
-        return builder.reset(Level.OFF);
+        final DefaultLogBuilder builder = (DefaultLogBuilder) recycler.acquire();
+        return builder.atLevel(Level.OFF);

Review Comment:
   ...so that we don't need this unchecked cast.
   
   I can open a new issue for this, since this PR is getting huge.



##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java:
##########
@@ -84,7 +82,8 @@ public abstract class AbstractLogger implements ExtendedLogger {
     private final MessageFactory messageFactory;
     private final FlowMessageFactory flowMessageFactory;
     private static final ThreadLocal<int[]> recursionDepthHolder = new ThreadLocal<>(); // LOG4J2-1518, LOG4J2-2031
-    protected final transient ThreadLocal<DefaultLogBuilder> logBuilder;
+    protected final Recycler<LogBuilder> recycler = LoggingSystem.getRecyclerFactory()
+            .create(() -> new DefaultLogBuilder(this));

Review Comment:
   As a future extension we should probably allow any `LogBuilder`...



##########
log4j-api/src/main/java/org/apache/logging/log4j/internal/DefaultLogBuilder.java:
##########
@@ -246,16 +231,15 @@ private void logMessage(final Message message) {
         try {
             logger.logMessage(level, marker, fqcn, location, message, throwable);
         } finally {
-            inUse = false;
+            // recycle self
+            this.level = null;
+            this.marker = null;
+            this.throwable = null;
+            this.location = null;

Review Comment:
   I fail to understand how can the recycler know that the `LogBuilder` can be used again?



-- 
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: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org