[jira] Issue Comment Edited: (LOGGING-137) LogFactory.getLog()

["Doug Bateman (JIRA)" <ji...@apache.org>]

    [ 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/8/10 11:56 PM:
---------------------------------------------------------------

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?

      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 tolerate in the event I was wrong.  I didn't make it volatile because I wanted to avoid the memory flush.  If a thread had 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.