commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Doug Bateman (JIRA)" <j...@apache.org>
Subject [jira] Issue Comment Edited: (LOGGING-137) LogFactory.getLog()
Date Mon, 09 Aug 2010 04:16:15 GMT

    [ https://issues.apache.org/jira/browse/LOGGING-137?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12896447#action_12896447
] 

Doug Bateman edited comment on LOGGING-137 at 8/9/10 12:15 AM:
---------------------------------------------------------------

Hey Sebastian,

Yeah, the patch file does have a spurious update to svn:ignore.  It adds ".settings/" to the
list.  Eclipse complains and that directory has been included in commons-lang and commons-io,
but overlooked in commons-logging.  However, you're right, that's really a separate issue.
 So consider it gone.

The .java files do duplicate the .patch file.  Normally I'd just give the .patch file, but
since we were having a discussion in channel I included the .java files too for ease of reading.
 For the update, which would you prefer... Java files for patch files?

I'll remove the @author tag.  I was about to take it out, but noticed all the other files
still had them.

Regarding the null return by CallStackUtil, it's really a non-issue since it can never happen.
 The only place getCallerClassName(1) is used is inside of LogFactory.getLog().  And LogFactory.getLog()
is guaranteed to have an immediate caller (depth=1), otherwise it never would have been called
in the first place.  Nobody else can use CallStackUtil because it is package scope... It didn't
seem relevent to the public API for logging... and if it was ever to be published, it probably
should be in commons-lang.  I suppose I could have just made private methods inside of LogFactory,
but I didn't want to add the clutter and package scope seemed appropriate.  That aside, the
JavaDoc does actually discuss meaning of a null return, however I'll update the docs to make
it a bit more obvious.

The non-final fields were strictly a matter of simplicity, since the compiler objects to a
possible double assignment to the file in the try and again in the catch.  It objects even
when using separate try/catch blocks for each set.  However, I can work around that by using
local variables in the initializer, so that's what I'll do.

CallStackUtil.getCallerJava14() doesn't actually scan the trace twice.  The i counter only
ever increases.  First it scans to find the invocation to the "CallStackUtil" methods on the
stack (to skip past the reflection calls), and then it scans further until it's past the "CallStackUtil"
methods.  Leaving the interesting part of the call stack, which is then read at the requested
depth.  getCallerJava10() actually works the same way.  It just uses "stackTrace.lastIndexOf()"
to accomplish this, since it's reading the stack as a string.

I too wanted to use Thread.getStackTrace().  However I noticed in the JavaDoc that it can
throw SecurityExceptions, while Throwable.getStackTrace() does not.  I have no idea why they
decided to allow that in one but not the other.  And of course Thread.getStackTrace() isn't
available until Java 5.0 instead of 1.4.  So following the "keep it simple" rule, I decided
to use Throwable.getStackTrace().  I don't imagine the performance difference is measurable
since Throwable.getStackTrace() likely uses the exact same native code underneath.  And the
extra object allocation is insignificant in comparision to the use of Java reflection.  If
after all this, you still think I should add support for Java 5.0's Thread.getStackTrace()
I'm happy to do add it.

I made isAtLeastJava14 non-volatile deliberately as well.  In actuality, it could probably
be made final, since I cant imagine a case where those catch blocks would ever be hit, except
maybe in the case of a SecurityException.  I had simply decided to make the implementation
fault tolerant in the event I was wrong.  I didn't make it volatile because I wanted to avoid
the memory flush.  If a thread has a stale copy of the flag no harm is done... that stale
copy would always have the value true (from the static initializer which is synchronized by
the classloader).  And that just means the catch blocks would be hit again, and that thread
would catch the exception and set it's cached copy to false as well... no memory flush required.
 I should probably comment that though, since it is unintuitive.

I'll submit an update.  Would you prefer it as java files or a single patch file?


      was (Author: dougbateman):
    Yeah, the patch file does have a spurious update to svn:ignore.  It adds ".settings/"
to the list.  Eclipse complains and that directory has been included in commons-lang and commons-io,
but overlooked in commons-logging.  However, you're right, that's really a separate issue.
 So consider it gone.

The .java files do duplicate the .patch file.  Normally I'd just give the .patch file, but
since we were having a discussion in channel I included the .java files too for ease of reading.
 For the update, which would you prefer... Java files for patch files?

And I'll update the JavaDoc.  CallStackUtil is package scope (for reasons I'll mention below),
so that null case really shouldn't be an issue (also for reasons I'll mention below).

And I'll remove the @author tag.  I was about to take it out, but noticed all the other files
still had them.

The non-final fields were strictly a matter of simplicity, since the compiler objects to a
possible double assignment to the file in the try and again in the catch.  It objects even
when using separate try/catch blocks for each set.  However, I can work around that by using
local variables in the initializer, so that's what I'll do.

CallStackUtil.getCallerJava14() doesn't actually scan the trace twice.  The i counter only
ever increases.  First it scans to find the invocation to the "CallStackUtil" methods on the
stack (to skip past the reflection calls), and then it scans further until it's past the "CallStackUtil"
methods.  Leaving the interesting part of the call stack, which is then read at the requested
depth.  getCallerJava10() actually works the same way.  It just uses "stackTrace.lastIndexOf()"
to accomplish this, since it's reading the stack as a string.

I too wanted to use Thread.getStackTrace().  However I noticed in the JavaDoc that it can
throw SecurityExceptions, while Throwable.getStackTrace() does not.  I have no idea why they
decided to allow that in one but not the other.  And of course Thread.getStackTrace() isn't
available until Java 5.0 instead of 1.4.  So following the "keep it simple" rule, I decided
to use Throwable.getStackTrace().  I don't imagine the performance difference is measurable
since Throwable.getStackTrace() likely uses the exact same native code underneath.  And the
extra object allocation is insignificant in comparision to the use of Java reflection.  If
after all this, you still think I should add support for Java 5.0's Thread.getStackTrace()
I'm happy to do add it.

I also made isAtLeastJava14 non-volatile deliberately as well.  In actuality, it could probably
be made final, since I cant imagine a case where those catch blocks would ever be hit, except
maybe in the case of a SecurityException.  I had simply decided to make the implementation
fault tolerant in the event I was wrong.  I didn't make it volatile because I wanted to avoid
the memory flush.  If a thread has a stale copy of the flag no harm is done... that stale
copy would always have the value true (from the static initializer which is synchronized by
the classloader).  And that just means the catch blocks would be hit again, and that thread
would catch the exception and set it's cached copy to false as well... no memory flush required.
 I should probably comment that though, since it is unintuitive.

As for a null return by CallStackUtil, it's really a non-issue since it can never happen (and
I should probably add an assertion for that).  CallStackUtil is package scope because it doesn't
really have anything to do with the public API for a logging package... and if it was ever
to be published, it probably should be in commons-lang.  I suppose I could have just made
private methods inside of LogFactory, but I didn't want to add the clutter and package scope
seemed appropriate.  Null can never be returned to the logger because LogFactory.getLog()
asks for its immediate caller, which is guaranteed to exist (and thus not be null).

I'll submit an update.  Would you prefer it as java files or a single patch file?
  
> LogFactory.getLog()
> -------------------
>
>                 Key: LOGGING-137
>                 URL: https://issues.apache.org/jira/browse/LOGGING-137
>             Project: Commons Logging
>          Issue Type: New Feature
>    Affects Versions: 1.1.2
>            Reporter: Doug Bateman
>         Attachments: CallStack.patch, CallStackTest.java, CallStackUtil.java, LogFactory.java
>
>   Original Estimate: 0.5h
>  Remaining Estimate: 0.5h
>
> Presently, in Apache Commons, the most common way to get a logger is to do something
like:
> public class MyClass {
>     private static Log log = LogFactory.getLog(MyClass.class);
> }
> Notice how MyClass.class (or alternatively a string name) is passed as a parameter. 
The annoying aspect of this is that sometimes the class name doesn't get updated when doing
copy/paste operations.  A desirable alternative might be:
> public class MyClass {
>     private static Log log = LogFactory.getLog(); //class name inferred from call stack
> }
> With such an approach there are two possible concerns I can foresee:
>     * Call stack inspection isn't terribly fast.  However since Loggers are generally
initialized only once, when the class is first loaded, performance isn't likely to be a major
problem.
>     * Commons-logging is Java 1.1 compatible.  Thus care must be taken to ensure compatibility
isn't broken.
>     * Commons-logging doesn't depend on commons-lang, and thus the utilities in commons-lang
cannot be used.
> In Java 1.4, the call stack is easily obtained using Thread.getCallStack().  Prior to
Java 1.4, the only way to obtain the call stack is to inspect the stack trace of an exception.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message