ramanathan1504 commented on code in PR #4404:
URL: https://github.com/apache/logging-log4j2/pull/4404#discussion_r4330002165
##########
log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java:
##########
@@ -363,14 +361,12 @@ public void log(
return;
}
final Message msg;
- final Throwable actualThrowable;
if (params == null) {
- msg = new SimpleMessage(message);
- actualThrowable = throwable;
+ msg = logger.getMessageFactory().newMessage(message);
} else {
- msg = new ParameterizedMessage(message, params, throwable);
- actualThrowable = throwable != null ? throwable :
msg.getThrowable();
+ msg = logger.getMessageFactory().newMessage(message, params);
}
+ final Throwable actualThrowable = throwable != null ? throwable :
msg.getThrowable();
logger.logMessage(fqcn, log4jLevel, log4jMarker, msg, actualThrowable);
Review Comment:
The default factory is `ReusableMessageFactory`, and `Logger.logMessage()`
does not release the message. Without this the thread-local message stays
reserved after the first call.
```suggestion
try {
logger.logMessage(fqcn, log4jLevel, log4jMarker, msg,
actualThrowable);
} finally {
ReusableMessageFactory.release(msg);
}
```
##########
log4j-slf4j-impl/src/test/java/org/apache/logging/slf4j/LoggerTest.java:
##########
@@ -49,6 +56,77 @@ public LoggerTest(final LoggerContext context) {
this.logger = LoggerFactory.getLogger("LoggerTest");
}
+ @Test
+ void locationAwareLoggerUsesConfiguredMessageFactory() {
+ final String name = "LoggerTest.CustomMessageFactory";
+ final Log4jLogger customLogger = new Log4jLogger(
+ new Log4jMarkerFactory(), ctx.getLogger(name,
StringFormatterMessageFactory.INSTANCE), name);
+
+ final ListAppender list = getAppenderByName("UnformattedList");
+ list.clear();
+
+ // Control: regular logging honors the configured message factory.
+ customLogger.info("value=%s", "normal");
+
+ assertEquals(1, list.getEvents().size());
+ assertEquals(
+ StringFormattedMessage.class,
+ list.getEvents().get(0).getMessage().getClass());
+ assertEquals("value=normal",
list.getEvents().get(0).getMessage().getFormattedMessage());
+
+ list.clear();
+
+ // Regression: location-aware logging must use the same factory.
+ customLogger.log(
+ null,
+ LoggerTest.class.getName(),
+ LocationAwareLogger.INFO_INT,
+ "value=%s",
+ new Object[] {"aware"},
+ null);
+
+ assertEquals(1, list.getEvents().size());
+ assertEquals(
+ StringFormattedMessage.class,
+ list.getEvents().get(0).getMessage().getClass());
+ assertEquals("value=aware",
list.getEvents().get(0).getMessage().getFormattedMessage());
+ }
+
+ @Test
+ void locationAwareLoggerUsesConfiguredMessageFactoryWithoutParameters() {
Review Comment:
No test reaches the `msg.getThrowable()` fallback or uses
`ReusableMessageFactory`. Can we add one that passes a trailing `Throwable` in
`params`, and one with `ReusableMessageFactory` that checks the message is
released?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
