commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From robert burrell donkin <rdon...@apache.org>
Subject Re: [logging] LoadTest
Date Mon, 30 May 2005 21:24:18 GMT
On Mon, 2005-05-30 at 18:40 +1200, Simon Kitching wrote:
> On Sun, 2005-05-29 at 21:17 -0700, Brian Stansberry wrote:
> > I've been testing my new implementation of
> > LogFactoryImpl and it's looking good except it fails
> > the "testInContainer" test of o.a.c.l.LoadTest.  This
> > test sets up a parent-last (aka child-first)
> > classloader hierarchy that has JCL and a class that
> > calls JCL in the child.  JCL is also visible in the
> > parent.  It then performs various tests, setting the
> > thread context class loader to various values and
> > trying to log.
> > 
> > This test is designed to fail if logging succeeds when
> > the TCCL is set to the system classloader or the
> > parent classloader.  
> > 
> > The old implementation of LogFactoryImpl does throw a
> > LogConfigurationException in this situation, as
> > discovery finds an adapter in the parent that is
> > binary incompatible with the LogFactoryImpl in the
> > child.
> 
> I suspect it actually fails because LogFactoryImpl finds Log4JLogger in
> the parent, but log4j classes are not in the parent. 

i think that this may depend on the classpath used to run the test.

> It is a fundamental
> implication of the logging design that the XXLogger must be able to load
> the library it maps to via normal java classloader lookup (ie no using
> the context classloader). It's not "binary incompatibility", I think,
> but a complete failure to link Log4JLogger to its referenced classes.

i suspect that this is a test for exotic context classloader
configurations. i think that the mode of failure depends on the system
classpath used to run the test. (for example, maybe it was intended that
log4j would be defined by the system classloader) unless the intended
configuration is known, i think it'd be hard to work out the exact
diagnosis...

> > The new implementation does not throw an LCE, as it
> > detects the incompatibility and retries loading the
> > adapter using the LogFactoryImpl's classloader.  But,
> > because logging succeeds, this unit test does not
> > pass.
> > 
> > It is my interpretation that this unit test is really
> > checking for *consistent* behavior of the old
> > implementation, not for *correct* behavior.  It would
> > catch things like a change to the old implementation
> > that caused it to forget to try the TCCL.  But IMHO an
> > implementation that was specifically designed to
> > handle this situation shouldn't cause a unit test
> > failure.
> 
> I agree that the test is not relevant to your new code. It was really
> documenting the behaviour of the old system, and could be updated.

i suspect that the majority opinion was that JCL should refuse to run in
containers which didn't obey the rules. therefore, this was a legitimate
test. however, the current consensus is that (with hindsight) this
original decision was wrong. it was a semantic bug. i really cannot
think that any code could reasonably rely on JCL throwing a runtime
exception whenever exotic context classloader configurations are
encountered.

i agree that it should be updated. 

> An updated test really ought to verify that when context=system, logging
> falls back to jdk14 (when running with java14) or to SimpleLog. But if
> this is too much work, just asserting that logging still works would be
> ok by me.

+1

> > BTW, LoadTest is not invoked by the ant test target.
> 
> Then that should probably be fixed too. Nicely spotted!

+1

- robert


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message