[
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.
