You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by "Simon Kitching (JIRA)" <ji...@apache.org> on 2006/11/19 10:04:39 UTC
[jira] Commented: (LOGGING-111) Small patch to make debugging
easier
[ http://issues.apache.org/jira/browse/LOGGING-111?page=comments#action_12451127 ]
Simon Kitching commented on LOGGING-111:
----------------------------------------
Looks good to me. I've committed this patch (with a couple of extra checks for null causes).
Thanks very much for your contribution.
SVN commit: 476772 (LogFactoryImpl), 476774 (Log4JLogger).
Could you please check that this works fine for you?
Note for others: InvocationTargetException.getTargetException and ExceptionInInitializerError.getException both exist in jdk1.2.2 (supported platform for JCL).
> Small patch to make debugging easier
> ------------------------------------
>
> Key: LOGGING-111
> URL: http://issues.apache.org/jira/browse/LOGGING-111
> Project: Commons Logging
> Issue Type: Improvement
> Affects Versions: 1.1.1
> Environment: Commons-Logging + Log4j
> Reporter: Lilianne E. Blaze
>
> During the last few days I had major problems trying to configure Commons-Logging + Log4j on Glassfish.
> It turned out to be related to Log4j UDPAppender, but it took me needlessly long time to verify the problem was indeed in Log4j and not in Commons-Logging, Glassfish or something else. Now, why am I posting it here then - I made a small modification which logs Log4j failures more precisely, instead of just:
> [LogFactoryImpl@14810083 from org.apache.catalina.loader.WebappClassLoader@1904280] Could not instantiate Log 'org.apache.commons.logging.impl.Log4JLogger' -- java.lang.reflect.InvocationTargetException: null
> which explains, basically, nothing, you get for example:
> [LogFactoryImpl@14810083 from org.apache.catalina.loader.WebappClassLoader@1904280] Could not instantiate Log 'org.apache.commons.logging.impl.Log4JLogger' -- java.lang.reflect.InvocationTargetException: null
> [LogFactoryImpl@14810083 from org.apache.catalina.loader.WebappClassLoader@1904280] ... InvocationTargetException: java.lang.ExceptionInInitializerError: null
> [LogFactoryImpl@14810083 from org.apache.catalina.loader.WebappClassLoader@1904280] ... ExceptionInInitializerError: java.lang.IllegalStateException: Property layout must be set for UDPAppender named appenderLocalhostUdp
> which states clearly that Log4j was indeed loaded, and the problem was in its configuration.
> All it does is expand those two exceptions if they occurred. It could be more general and more elegant, but this code should work in pre-1.4 Java.
> Could you please include it in next build of Commons-Logging?
> Attaching the patch text below.
> Greetings, Lilianne E. Blaze
> Index: LogFactoryImpl.java
> *** D:\Work\Projects\Apache\commons-logging-custom\src\org\apache\commons\logging\impl\LogFactoryImpl.java Base (BASE)
> --- D:\Work\Projects\Apache\commons-logging-custom\src\org\apache\commons\logging\impl\LogFactoryImpl.java Locally Modified (Based On LOCAL)
> ***************
> *** 1362,1369 ****
> --- 1362,1388 ----
> + logAdapterClassName + "' -- "
> + discoveryFlaw.getClass().getName() + ": "
> + discoveryFlaw.getLocalizedMessage());
> + + if ( discoveryFlaw instanceof InvocationTargetException ) {
> + InvocationTargetException ite = (InvocationTargetException)discoveryFlaw;
> + Throwable cause = ite.getTargetException();
> + logDiagnostic("... InvocationTargetException: " +
> + cause.getClass().getName() + ": " +
> + cause.getLocalizedMessage());
> + + if( cause instanceof ExceptionInInitializerError ) {
> + ExceptionInInitializerError eiie = (ExceptionInInitializerError)cause;
> + Throwable cause2 = eiie.getException();
> + logDiagnostic("... ExceptionInInitializerError: " +
> + cause2.getClass().getName() + ": " +
> + cause2.getLocalizedMessage());
> + }
> + }
> + + }
> + if (!allowFlawedDiscovery) {
> throw new LogConfigurationException(discoveryFlaw);
> }
> Index: Log4JLogger.java
> *** D:\Work\Projects\Apache\commons-logging-custom\src\org\apache\commons\logging\impl\Log4JLogger.java Base (BASE)
> --- D:\Work\Projects\Apache\commons-logging-custom\src\org\apache\commons\logging\impl\Log4JLogger.java Locally Modified (Based On LOCAL)
> ***************
> *** 77,84 ****
> --- 77,86 ----
> // ------------------------------------------------------------
> static {
> +
> if (!Priority.class.isAssignableFrom(Level.class)) {
> // nope, this is log4j 1.3, so force an ExceptionInInitializerError
> + // note - it still works with log4j 1.3.8-alpha
> throw new InstantiationError("Log4J 1.2 not available");
> }
> ***************
> *** 112,117 ****
> --- 114,124 ----
> /** For use with a log4j factory.
> */
> public Log4JLogger(Logger logger ) {
> + + if( logger == null ) {
> + throw new IllegalArgumentException("Warning - logger == null, possible Log4j misconfiguration?");
> + }
> + this.name = logger.getName();
> this.logger=logger;
> }
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [jira] Commented: (LOGGING-111) Small patch to make debugging
easier
Posted by "Lilianne E. Blaze" <li...@tlen.pl>.
Simon Kitching wrote:
>> I noticed some small differences between my patch and your commits, was
>> it just a matter of code formatting conventions, or did I do something
>> wrong? Asking just in case.
>>
>
> The original patch wasn't checking for null in some places, eg
> Throwable cause = ite.getTargetException();
> logDiagnostic("... InvocationTargetException: " +
> cause.getClass().getName() + ": " +
> cause.getLocalizedMessage());
> would throw NullPointerException if cause is null. Now I don't know
> whether it ever is null in practice, but InvocationTargetException has a
> default constructor that will set it to null. It seemed safest to check.
>
As I understand it those two exception types (InvocationTarget and
ExceptionInInitializer) are meant to always wrap the 'real' exceptions,
so cause and cause2 shouldn't be null in any circumstances. Of course,
"shouldn't" != "can't".
So yes, I agree, you can't be too cautious. And since it's one-time
event there's no point in over-optimizing by skipping the checks.
> And by the way, an attached patch file would have been more convenient
> than pasting the patch text inline, though for a small patch like this
> it wasn't a big deal.
>
>
>> Also, the "// note - it still works with log4j 1.3.8-alpha" comment was
>> removed? I know it wasn't strictly a part of this enhancement, but isn't
>> it worth mentioning, especially since Log4j 1.3 is in .7 - .8 alpha for
>> a long time now and probably will be for some time?
>>
>
> The phrase "it still works" seemed ambiguous to me in this case. Does it
> mean the check passes (and should), passes (and shouldn't) or something
> else? I couldn't think of a better way to put it though, and didn't have
> time to research what the log4j people are intending for the next
> release so left that comment out. Possibly I could have done better
> here, but I'm just really busy at the moment...
>
I think that would be "something else". In a couple of places
documentation and source code mentions some class inheritance changes in
Log4j which are supposed to make Log4JLogger incompatible with Log4j
1.3. But I've been using Commons-Logging for a long time now with Log4j
1.3.3 alpha, 1.3.6, 1.3.7 and current CVS versions and I haven't noticed
any such incompatibilities. Note the test:
if (!Priority.class.isAssignableFrom(Level.class)) {
// nope, this is log4j 1.3, so force an
ExceptionInInitializerError
throw new InstantiationError("Log4J 1.2 not available");
}
Is supposed to differentiate between 1.3 and 1.2 by
Priority.class.isAssignableFrom(Level.class) expression.
It is a "shouldn't" because it 'thinks' 1.3.8-alpha is 1.2.x, but it is
a "should" because 1.3.8-alpha is still compatible with 1.2.x.
How about:
// note - this test incorrectly detects Log4j versions up to and
including 1.3.7-alpha
// and up to and including SVN 2006.11.17 as Log4j 1.2.x, but these
versions seem to
// still be compatible with code designed to work with Log4j 1.2.x
Unless of course there are incompatibilities, and I just haven't run
into them yet? (Most of the time I just use a pretty basic configuration
with logging to one or more rolling files and one or two remote hosts).
> Thanks very much for your patch; I'm sure it will save users some
> frustration (maybe even me).
>
> Regards,
>
> Simon
>
Greetings, Lilianne E. Blaze
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [jira] Commented: (LOGGING-111) Small patch to make debugging
easier
Posted by Simon Kitching <sk...@apache.org>.
On Sun, 2006-11-19 at 12:19 +0100, Lilianne E. Blaze wrote:
> Simon Kitching (JIRA) wrote:
> > [ http://issues.apache.org/jira/browse/LOGGING-111?page=comments#action_12451127 ]
> >
> > Simon Kitching commented on LOGGING-111:
> > ----------------------------------------
> >
> > Looks good to me. I've committed this patch (with a couple of extra checks for null causes).
> > Thanks very much for your contribution.
> >
> > SVN commit: 476772 (LogFactoryImpl), 476774 (Log4JLogger).
> >
> > Could you please check that this works fine for you?
> >
>
> Yes, thank you.
Great. I'll close the issue.
>
> I noticed some small differences between my patch and your commits, was
> it just a matter of code formatting conventions, or did I do something
> wrong? Asking just in case.
The original patch wasn't checking for null in some places, eg
Throwable cause = ite.getTargetException();
logDiagnostic("... InvocationTargetException: " +
cause.getClass().getName() + ": " +
cause.getLocalizedMessage());
would throw NullPointerException if cause is null. Now I don't know
whether it ever is null in practice, but InvocationTargetException has a
default constructor that will set it to null. It seemed safest to check.
And by the way, an attached patch file would have been more convenient
than pasting the patch text inline, though for a small patch like this
it wasn't a big deal.
> Also, the "// note - it still works with log4j 1.3.8-alpha" comment was
> removed? I know it wasn't strictly a part of this enhancement, but isn't
> it worth mentioning, especially since Log4j 1.3 is in .7 - .8 alpha for
> a long time now and probably will be for some time?
The phrase "it still works" seemed ambiguous to me in this case. Does it
mean the check passes (and should), passes (and shouldn't) or something
else? I couldn't think of a better way to put it though, and didn't have
time to research what the log4j people are intending for the next
release so left that comment out. Possibly I could have done better
here, but I'm just really busy at the moment...
Thanks very much for your patch; I'm sure it will save users some
frustration (maybe even me).
Regards,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [jira] Commented: (LOGGING-111) Small patch to make debugging
easier
Posted by "Lilianne E. Blaze" <li...@tlen.pl>.
Simon Kitching (JIRA) wrote:
> [ http://issues.apache.org/jira/browse/LOGGING-111?page=comments#action_12451127 ]
>
> Simon Kitching commented on LOGGING-111:
> ----------------------------------------
>
> Looks good to me. I've committed this patch (with a couple of extra checks for null causes).
> Thanks very much for your contribution.
>
> SVN commit: 476772 (LogFactoryImpl), 476774 (Log4JLogger).
>
> Could you please check that this works fine for you?
>
Yes, thank you.
I noticed some small differences between my patch and your commits, was
it just a matter of code formatting conventions, or did I do something
wrong? Asking just in case.
Also, the "// note - it still works with log4j 1.3.8-alpha" comment was
removed? I know it wasn't strictly a part of this enhancement, but isn't
it worth mentioning, especially since Log4j 1.3 is in .7 - .8 alpha for
a long time now and probably will be for some time?
Greetings, Lilianne E. Blaze
> Note for others: InvocationTargetException.getTargetException and ExceptionInInitializerError.getException both exist in jdk1.2.2 (supported platform for JCL).
>
>
>> Small patch to make debugging easier
>> ------------------------------------
>>
>> Key: LOGGING-111
>> URL: http://issues.apache.org/jira/browse/LOGGING-111
>> Project: Commons Logging
>> Issue Type: Improvement
>> Affects Versions: 1.1.1
>> Environment: Commons-Logging + Log4j
>> Reporter: Lilianne E. Blaze
>>
>> During the last few days I had major problems trying to configure Commons-Logging + Log4j on Glassfish.
>> It turned out to be related to Log4j UDPAppender, but it took me needlessly long time to verify the problem was indeed in Log4j and not in Commons-Logging, Glassfish or something else. Now, why am I posting it here then - I made a small modification which logs Log4j failures more precisely, instead of just:
>> [LogFactoryImpl@14810083 from org.apache.catalina.loader.WebappClassLoader@1904280] Could not instantiate Log 'org.apache.commons.logging.impl.Log4JLogger' -- java.lang.reflect.InvocationTargetException: null
>> which explains, basically, nothing, you get for example:
>> [LogFactoryImpl@14810083 from org.apache.catalina.loader.WebappClassLoader@1904280] Could not instantiate Log 'org.apache.commons.logging.impl.Log4JLogger' -- java.lang.reflect.InvocationTargetException: null
>> [LogFactoryImpl@14810083 from org.apache.catalina.loader.WebappClassLoader@1904280] ... InvocationTargetException: java.lang.ExceptionInInitializerError: null
>> [LogFactoryImpl@14810083 from org.apache.catalina.loader.WebappClassLoader@1904280] ... ExceptionInInitializerError: java.lang.IllegalStateException: Property layout must be set for UDPAppender named appenderLocalhostUdp
>> which states clearly that Log4j was indeed loaded, and the problem was in its configuration.
>> All it does is expand those two exceptions if they occurred. It could be more general and more elegant, but this code should work in pre-1.4 Java.
>> Could you please include it in next build of Commons-Logging?
>> Attaching the patch text below.
>> Greetings, Lilianne E. Blaze
>> Index: LogFactoryImpl.java
>> *** D:\Work\Projects\Apache\commons-logging-custom\src\org\apache\commons\logging\impl\LogFactoryImpl.java Base (BASE)
>> --- D:\Work\Projects\Apache\commons-logging-custom\src\org\apache\commons\logging\impl\LogFactoryImpl.java Locally Modified (Based On LOCAL)
>> ***************
>> *** 1362,1369 ****
>> --- 1362,1388 ----
>> + logAdapterClassName + "' -- "
>> + discoveryFlaw.getClass().getName() + ": "
>> + discoveryFlaw.getLocalizedMessage());
>> + + if ( discoveryFlaw instanceof InvocationTargetException ) {
>> + InvocationTargetException ite = (InvocationTargetException)discoveryFlaw;
>> + Throwable cause = ite.getTargetException();
>> + logDiagnostic("... InvocationTargetException: " +
>> + cause.getClass().getName() + ": " +
>> + cause.getLocalizedMessage());
>> + + if( cause instanceof ExceptionInInitializerError ) {
>> + ExceptionInInitializerError eiie = (ExceptionInInitializerError)cause;
>> + Throwable cause2 = eiie.getException();
>> + logDiagnostic("... ExceptionInInitializerError: " +
>> + cause2.getClass().getName() + ": " +
>> + cause2.getLocalizedMessage());
>> + }
>> + }
>> + + }
>> + if (!allowFlawedDiscovery) {
>> throw new LogConfigurationException(discoveryFlaw);
>> }
>> Index: Log4JLogger.java
>> *** D:\Work\Projects\Apache\commons-logging-custom\src\org\apache\commons\logging\impl\Log4JLogger.java Base (BASE)
>> --- D:\Work\Projects\Apache\commons-logging-custom\src\org\apache\commons\logging\impl\Log4JLogger.java Locally Modified (Based On LOCAL)
>> ***************
>> *** 77,84 ****
>> --- 77,86 ----
>> // ------------------------------------------------------------
>> static {
>> +
>> if (!Priority.class.isAssignableFrom(Level.class)) {
>> // nope, this is log4j 1.3, so force an ExceptionInInitializerError
>> + // note - it still works with log4j 1.3.8-alpha
>> throw new InstantiationError("Log4J 1.2 not available");
>> }
>> ***************
>> *** 112,117 ****
>> --- 114,124 ----
>> /** For use with a log4j factory.
>> */
>> public Log4JLogger(Logger logger ) {
>> + + if( logger == null ) {
>> + throw new IllegalArgumentException("Warning - logger == null, possible Log4j misconfiguration?");
>> + }
>> + this.name = logger.getName();
>> this.logger=logger;
>> }
>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org