From fa0110e442649a2bda88244bbe629c39a766076b Mon Sep 17 00:00:00 2001 From: William Thurston Date: Sat, 14 Oct 2017 23:21:35 -0700 Subject: [PATCH] Address deadlock when attempting to avoid stack creation. --- .../com/netflix/blitz4j/LoggingContext.java | 104 ++++------------- .../logging/log4jAdapter/NFPatternParser.java | 108 +++++++++--------- 2 files changed, 75 insertions(+), 137 deletions(-) diff --git a/src/main/java/com/netflix/blitz4j/LoggingContext.java b/src/main/java/com/netflix/blitz4j/LoggingContext.java index 12e93a3..7f523d2 100644 --- a/src/main/java/com/netflix/blitz4j/LoggingContext.java +++ b/src/main/java/com/netflix/blitz4j/LoggingContext.java @@ -16,24 +16,16 @@ package com.netflix.blitz4j; -import com.netflix.logging.log4jAdapter.NFPatternLayout; import com.netflix.servo.monitor.Monitors; import com.netflix.servo.monitor.Stopwatch; import com.netflix.servo.monitor.Timer; -import org.apache.log4j.Appender; import org.apache.log4j.Category; import org.apache.log4j.Level; import org.apache.log4j.MDC; -import org.apache.log4j.spi.AppenderAttachable; import org.apache.log4j.spi.LocationInfo; import org.apache.log4j.spi.LoggingEvent; -import java.util.Collections; -import java.util.Enumeration; import java.util.HashSet; -import java.util.Set; -import java.util.WeakHashMap; -import java.util.concurrent.CopyOnWriteArraySet; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; @@ -170,7 +162,7 @@ public LocationInfo generateLocationInfo(LoggingEvent event) { try { // We should only generate location info if the caller is using NFPatternLayout otherwise this is expensive and unused. if (isUsingNFPatternLayout(event.getLogger())) { - locationInfo = (LocationInfo) LoggingContext + locationInfo = LoggingContext .getInstance() .getLocationInfo(Class.forName(event.getFQNOfLoggerClass())); if (locationInfo != null) { @@ -186,85 +178,31 @@ public LocationInfo generateLocationInfo(LoggingEvent event) { return locationInfo; } - private boolean isUsingNFPatternLayout(Category logger) { - if (logger == null) { - return false; - } - + public void shouldGenerateLocationInfo(Category logger) { HashSet loggerNeedsLocation = loggerNeedsLocationRef.get(); - // If we've already seen this logger and it needs location info, assume it still does. - // Due to reconfiguration, it's possible it doesn't anymore, but this is rare so we optimize - // for a fast return on loggers previously known to need location info. - if (loggerNeedsLocation.contains(logger)) { - return true; - } - - // If any of the appenders in the tree below need location information remember this logger and return. - if (isUsingNFPatternLayout(logger.getAllAppenders())) { - do { - HashSet copy = new HashSet<>(loggerNeedsLocation); - copy.add(logger); - if (loggerNeedsLocationRef.compareAndSet(loggerNeedsLocation, copy)) { - return true; - } - loggerNeedsLocation = loggerNeedsLocationRef.get(); - } while(true); - } - - // If this is not an additive logger, our search is done, otherwise we must look at parents. - if (!logger.getAdditivity()) { - return false; - } - - Category parentLogger = logger.getParent(); - if (parentLogger == null) { - return false; - } - - // Now we need to traverse all parents and remember the top level logger whose parents need - // location info if additivity was set to true. - if(isUsingNFPatternLayout(parentLogger)) { - do { - HashSet copy = new HashSet<>(loggerNeedsLocation); - copy.add(logger); - if (loggerNeedsLocationRef.compareAndSet(loggerNeedsLocation, copy)) { - return true; - } - loggerNeedsLocation = loggerNeedsLocationRef.get(); - } while(true); - } - - // An exhaustive search returned nothing. We want location information to show up when - // a reconfiguration occurs so we don't cache the result. This is still a much cheaper - // cost than generating a stack trace and so we are happy to pay it every time we log - // if we don't actually need the location information. - return false; - - } - private boolean isUsingNFPatternLayout(Enumeration enumeration) { - if (enumeration == null) { - return false; - } - - while(enumeration.hasMoreElements()) { - Object maybeAppender = enumeration.nextElement(); - if (maybeAppender instanceof Appender) { - Appender a = (Appender) maybeAppender; - if (a.getLayout() instanceof NFPatternLayout) { - return true; - } + // Add the logger to the set of loggers that needs location info. + do { + // If we've already seen this logger return immediately. + if (loggerNeedsLocation.contains(logger)) { + return; } - if (maybeAppender instanceof AppenderAttachable) { - AppenderAttachable aa = (AppenderAttachable) maybeAppender; - if (isUsingNFPatternLayout(aa.getAllAppenders())) { - return true; - } + // Try to add the logger + HashSet copy = new HashSet<>(loggerNeedsLocation); + copy.add(logger); + if (loggerNeedsLocationRef.compareAndSet(loggerNeedsLocation, copy)) { + return; } - } - return false; + // If there's a conflict, pull the map out and try again. + loggerNeedsLocation = loggerNeedsLocationRef.get(); + } while(true); + } + + private boolean isUsingNFPatternLayout(Category logger) { + // Assume we don't need location info until proven otherwise + return logger != null && loggerNeedsLocationRef.get().contains(logger); } /** @@ -311,7 +249,7 @@ public void clearContextLevel() { /** * Get the context {@link Level} for the request-based logging - * @param level - The level of logging to be enabled for this request + * @return level - The level of logging to be enabled for this request */ public Level getContextLevel() { return (Level)MDC.get(CONTEXT_LEVEL); diff --git a/src/main/java/com/netflix/logging/log4jAdapter/NFPatternParser.java b/src/main/java/com/netflix/logging/log4jAdapter/NFPatternParser.java index 220d51a..2ce0eb8 100644 --- a/src/main/java/com/netflix/logging/log4jAdapter/NFPatternParser.java +++ b/src/main/java/com/netflix/logging/log4jAdapter/NFPatternParser.java @@ -33,66 +33,66 @@ * as class, line number etc. * * @author Karthik Ranganathan - * */ public class NFPatternParser extends PatternParser { - private static List contextCharList = Arrays.asList(Character.valueOf('c'), - Character.valueOf('l'), - Character.valueOf('M'), - Character.valueOf('C'), - Character.valueOf('L'), - Character.valueOf('F')); + private static List contextCharList = Arrays.asList(Character.valueOf('c'), + Character.valueOf('l'), + Character.valueOf('M'), + Character.valueOf('C'), + Character.valueOf('L'), + Character.valueOf('F')); + + public NFPatternParser(String pattern) { + super(pattern); + + } + + protected void finalizeConverter(char c) { + if (contextCharList.contains(Character.valueOf(c))) { + PatternConverter pc = new NFPatternConverter(formattingInfo, c); + addConverter(pc); + currentLiteral.setLength(0); + } else { + super.finalizeConverter(c); + } + } - public NFPatternParser(String pattern) { - super(pattern); - - } + private static class NFPatternConverter extends PatternConverter { + private char type; - protected void finalizeConverter(char c) { - if (contextCharList.contains(Character.valueOf(c))) { - PatternConverter pc = new NFPatternConverter(formattingInfo, c); - addConverter(pc); - currentLiteral.setLength(0); - } else { - super.finalizeConverter(c); - } - } + NFPatternConverter(FormattingInfo formattingInfo, char type) { + super(formattingInfo); + this.type = type; + } - private static class NFPatternConverter extends PatternConverter { - private char type; + @Override + public String convert(LoggingEvent event) { + LoggingContext.getInstance().shouldGenerateLocationInfo(event.getLogger()); + LocationInfo locationInfo = LoggingContext.getInstance().getLocationInfo(event); + if (locationInfo == null) { + return ""; + } + switch (type) { + case 'M': + return locationInfo.getMethodName(); + case 'c': + return event.getLoggerName(); + case 'C': + return locationInfo.getClassName(); + case 'L': + return locationInfo.getLineNumber(); + case 'l': + return (locationInfo.getFileName() + ":" + + locationInfo.getClassName() + " " + + locationInfo.getLineNumber() + " " + locationInfo + .getMethodName()); + case 'F': + return locationInfo.getFileName(); + } + return ""; - NFPatternConverter(FormattingInfo formattingInfo, char type) { - super(formattingInfo); - this.type = type; - } + } - @Override - public String convert(LoggingEvent event) { - LocationInfo locationInfo = LoggingContext.getInstance().getLocationInfo(event); - if (locationInfo == null) { - return ""; - } - switch (type) { - case 'M': - return locationInfo.getMethodName(); - case 'c': - return event.getLoggerName(); - case 'C': - return locationInfo.getClassName(); - case 'L': - return locationInfo.getLineNumber(); - case 'l': - return (locationInfo.getFileName() + ":" - + locationInfo.getClassName() + " " - + locationInfo.getLineNumber() + " " + locationInfo - .getMethodName()); - case 'F': - return locationInfo.getFileName(); - } - return ""; - - } - - } + } } \ No newline at end of file