[
https://issues.apache.org/jira/browse/LOG4J2-555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13944058#comment-13944058
]
Gary Gregory edited comment on LOG4J2-555 at 3/22/14 2:12 PM:
--------------------------------------------------------------
Thank you for the review [~rem...@yahoo.com]. These are all good points.
WRT
{quote}
in AbstractLogger the debug/trace/info/...(Object objMessage) methods now call
log(FQCN, Level.XXX, null, messageFactory.newMessage(message), null); so a
Message object may be created unnecessarily if isEnabled later returns false
{quote}
I wonder if we should fix this. On one hand:
This would have to be fixed because creating objects for nothing will be a
giant GC hit, to be considered a leak IMO.
OTOH, this would assume the call site is not guarded with a
{{isDebugEnabled()}} for example, so the 'leak' can be fixed by adding the
guard clause.
It depends what we define the contract for this method and class to be WRT to
bullet proofing against this kind of 'leak'.
The code with patch is:
{code:java}
/**
* Logs a message object with the {@link Level#DEBUG DEBUG} level.
*
* @param message the message object to log.
*/
@Override
public void debug(final Object message) {
log(FQCN, Level.DEBUG, null, messageFactory.newMessage(message), null);
}
{code}
The obvious fix is:
{code:java}
public void debug(final Object message) {
if (isEnabled(Level.DEBUG)) {
log(FQCN, Level.DEBUG, null, messageFactory.newMessage(message),
null);
}
}
{code}
But we have both:
{code:java}
org.apache.logging.log4j.spi.AbstractLogger.log(String, Level, Marker, Message,
Throwable)
org.apache.logging.log4j.spi.AbstractLogger.log(String, Level, Marker, Object,
Throwable)
{code}
Why not:
{code:java}
@Override
public void debug(final Object message) {
log(FQCN, Level.DEBUG, null, message, null);
}
{code}
?
A local build with the patch and this change builds OK with 'mvn test'.
was (Author: garydgregory):
Thank you for the review [~rem...@yahoo.com]. These are all good points.
WRT
{quote}
in AbstractLogger the debug/trace/info/...(Object objMessage) methods now call
log(FQCN, Level.XXX, null, messageFactory.newMessage(message), null); so a
Message object may be created unnecessarily if isEnabled later returns false
> Location-based functionality broken in AbstractLoggerWrapper subclasses
> -----------------------------------------------------------------------
>
> Key: LOG4J2-555
> URL: https://issues.apache.org/jira/browse/LOG4J2-555
> Project: Log4j 2
> Issue Type: Bug
> Components: API, Core
> Affects Versions: 2.0-rc1
> Reporter: Remko Popma
> Assignee: Remko Popma
> Fix For: 2.0-rc2
>
> Attachments: LOG4J2-555-delegate.patch, log4j2-555-bbrouwer-2.patch,
> log4j2-555-bbrouwer.patch
>
>
> *How to reproduce*
> * Create a custom logger that extends {{AbstractLoggerWrapper}} (or generate
> one with the tool attached to LOG4J2-519)
> * In the custom logger provide a public method that invokes the {{log(Level,
> String)}} method
> * Configure a pattern layout that uses location, like %C for the logger FQCN
> * From a sample app, call the public method on your custom logger.
> * The output will show the class name of the custom logger instead of the
> class name of the calling class in the sample application.
> *Cause*
> {{AbstractLogger}}'s FQCN field is {{static final}} and initialized to
> {{AbstractLogger.class.getName()}}. Then, in
> {{Log4jLogEvent#calcLocation()}}, when walking over the stack trace elements,
> the element _following_ the FQCN is returned. So only loggers that directly
> subclass from {{AbstractLogger}} will work correctly. Loggers that inherit
> from {{AbstractLoggerWrapper}} are two levels removed from {{AbstractLogger}}
> and the {{calcLocation()}} method will not work correctly.
> *Solution*
> I think {{AbstractLogger}}'s FQCN field should be made non-static, and
> initialized to {{getClass().getName()}} in the constructor of
> {{AbstractLogger}}. {{Log4jLogEvent#calcLocation()}} can then be modified to
> return the {{StackElement}} whose class name matches the FQCN, instead of the
> next element. Location-based functionality should then work for arbitrarily
> deep subclass hierarchies of AbstractLogger.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org
For additional commands, e-mail: log4j-dev-h...@logging.apache.org
