Progress on JCL (WAS [logging] proposal)
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Tue, 2005-08-02 at 20:00 +1200, Simon Kitching wrote: Note, however, that commons-logging isn't making much progress at the moment, and several issues standing in the way of a new release. So there's no guarantee of when the next release might actually be pushed out. i hope to have a bit more time now but i've now lost track of where we are Been in China with only a dial-up connection that was so slow as to make following this list impossible. Hope to be a little more active now that I'm back (can't be any less active than I have been). But, have also lost track of where we are). Changing jobs in a couple weeks; after a month hope to have much more time to help out. -- Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] JCL in webapps disabled in JBoss [WAS] requirements and static binding
--- Simon Kitching [EMAIL PROTECTED] wrote: On Mon, 2005-06-20 at 22:46 -0700, Brian Stansberry wrote: snip Of course because the error is now suppressed, people won't get any hint that the logging output is getting diverted to an unexpected destination - unless they explicitly use a commons-logging.properties file in their webapp to: * point to a specific logging adapter class, or * turn off ALLOW_FLAWED_LOG_HIERARCHY Since log4j is on the classpath, they'd have to do use commons-logging.properties anyway. Well, I have a patch to propose which will change that :-) Related to the comment you just added to LogFactoryImpl? But the issue is still that users can get the *wrong copy of log4j*. If they get the one in their webapp, then it will use a log4j.properties or log4j.xml file in the webapp to configure itself. If they get the one in the server, then it will have already configured itself from the log4j.properties or log4j.xml file in the server classpath and will totally ignore the webapp's config file. And if code in the webapp gets the webapp copy of log4j but code in the shared classpath called on behalf of the webapp (ie with context classloader set) gets the server copy of log4j then logging from those calls will go to the server. That's specifically why it's good to try to load the log adapter from the context classpath; if it gets log4j from there then logging by shared classes operating on behalf of a webapp use the log4j classes which were configured via a webapp-specific config file. Recent versions of log4j support this thing called a context hierarchy which means that even when deployed in a shared classpath, log4j will try to use config files from the context classpath. This will resolve the issue - but requires the container to set this all up first. Do you have any idea whether JBoss uses this log4j feature? Thanks for pointing this out. I while back I dug into the log4j code and saw them using the context classpath. Didn't know it was a new thing. To answer your question, at least beginning with JBoss 4.0.1 they use it. I know because the app I work on at my job uses it to separate our logging from the server's. (We put a log4j.xml in WEB-INF/classes). In JBoss 3.2.5 including log4j.xml in the webapp does not work. And now I know why. Hmm..given that JBoss filters out all jcl classes by default, isn't this painfully obvious with jboss? How on earth is a webapp supposed to configure its JCL logging when running inside jboss?? Surely any log4j.properties or log4j.xml file inside WEB-INF/classes is completely ignored in jboss (though picked up in tomcat)? The filtering of JCL is a new thing (added in 4.0.2) and in the 4.0 series configuring log4j from the context classpath works. Last night I also noticed avalon-framework.jar is on the server classpath. Don't know why, but may have something to do with making it available to shared classes if people specify it in a webapp. Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] JCL in webapps disabled in JBoss [WAS] requirements and static binding
--- Simon Kitching [EMAIL PROTECTED] wrote: Hi Brian, On Sun, 2005-06-19 at 16:22 -0700, Brian Stansberry wrote: Had a chance to check this out, and the problem is because JBoss/Tomcat uses commons-logging.jar instead of the commons-logging-api.jar used by standalone Tomcat. When a webapp is deployed JBoss also deploys container code that executes when the webapps classloader is the TCCL (e.g. a valve that helps support the JACC spec). This code fails with LogConfigurationExceptions due to incompatible Log interfaces. JBoss can't just use commons-logging-api.jar as they use log4j for server logging. Yep. Normally, a tomcat adminstrator will also replace the commons-logging-api.jar with the full jar in order to get better control over tomcat's output. Good news is, the version of JCL in svn fixes this problem :) Well, I don't think it should be regarded as such great news (though it's what we deliberately implemented :). Maybe a little bit good? ;-) With the latest JCL I was able to remove the JBoss filter of JCL and get logkit logging working for both webapp classes and Tomcat's JSPServlet by following the standard steps -- commons-logging.properties in WEB-INF/classes and logkit jar in the server classpath (thus visible to the JSPServlet). And of course if I just used log4j everything ran fine. What this means is that logging performed by shared classes and jboss framework classes isn't going to the logging destination configured in the webapp, but instead the one configured for the container. So while the app will actually run, there is still a major issue if the user really wants/expects that output in their webapp's standard log destination. The correct fix would be to deploy commons-logging-adapters.jar in the webapp instead of commons-logging.jar; then the conflict between Log classes wouldn't occur and they would get both a working app *and* any output generated by jboss/shared classes when context-classpath is set to the webapp would go to the webapp log destination. Yep. At some point I'll lobby the JBoss folks to change their filter so it only blocks LogFactory and Log. This would have the same effect as having users deploy commons-logging-adapters.jar. I doubt they'll remove the filter completely with so many copies of JCL 1.0.4 or earlier out there. BTW, most JBoss framework code will only log to Log4j. Most JBoss code uses an internal logging abstraction that discovers log4j if it's on the classpath (which it always is inside the appserver). There are a few modules that use JCL because they are supported as standalone products that can run outside the appserver. It was these modules that failed without the JCL patch you just applied. Of course because the error is now suppressed, people won't get any hint that the logging output is getting diverted to an unexpected destination - unless they explicitly use a commons-logging.properties file in their webapp to: * point to a specific logging adapter class, or * turn off ALLOW_FLAWED_LOG_HIERARCHY Since log4j is on the classpath, they'd have to do use commons-logging.properties anyway. I can live with that though; we can just make it point#1 in the new jcl FAQ that having a commons-logging.properties file specifying an explicit logadapter is a very good idea, and the very first thing to do when anything unexpected is happening. +1 And at least users who don't care about logging aren't brought to a grinding halt. +1 snip But, they'll probably still keep filtering JCL once a new version of JCL is out in order to keep apps still using old versions from blowing up. Sorry, I understood your comments to say that they aren't really treating JCL specially, it is just a side-effect of their Unified classloader stuff (which sounds a really bad idea to me, by the way). No, the ULR issue was unrelated. If JCL 1.04 is used and the filter is disabled, webapp deployment blows up when JBoss tries to deploy a custom valve. This valve subclasses Tomcat's ValveBase, so deep in the bowels of the code a call is made to LogFactory.getLog(). This call blows up due to incompatible Log interfaces. Could you clarify these when you get a moment? * are they treating JCL special (refusing loading from webapp)? Yes. Their webapp classloader specifically prevents loading of JCL from the webapp. Nothing else is blocked (although the filter is XML configurable to block other packages). * if so, which specific classes are they treating special? Everything under org.apache.commons.logging * which versions of jboss have this unified classloader stuff? The 4.0 and 3.2 series and I believe 3.0 as well. BTW, in their nightly builds they've started using a patched version of JCL whose LogFactory uses a WeakHashMap instead of a Hashtable. This fixes memory leak problems in their unit tests
[logging] JCL in webapps disabled in JBoss [WAS] requirements and static binding
--- Brian Stansberry [EMAIL PROTECTED] wrote: --- robert burrell donkin [EMAIL PROTECTED] wrote: On Thu, 2005-05-05 at 23:12 +1200, Simon Kitching wrote: On Wed, 2005-05-04 at 23:37 -0700, Brian Stansberry wrote: snip 2) When checking into the above, I discovered that in the latest JBoss, their webapp classloader won't load commons-logging.jar from WEB-INF/lib even if parent-last loading is in effect. It's specifically disabled. Seems there were type conflicts with JCL classes loaded by the integrated Tomcat container. Not sure yet what this is all about, but in any case the net effect is that as far as JCL is concerned, a webapp on JBoss behaves as if parent-first loading were in effect. InterestingI wonder if they posted any questions to the JCL development list before adopting this (apparently fairly radical) solution. I'll go have a look. seems like an odd solution. any more information on this? Not too much. The JBoss CVS commit history and their JIRA don't give too many details. For JBoss 4.0.2 they switched to using the standard Tomcat webapp classloader by default (instead of their own). I'm *guessing* that the problem might arise because JBoss has commons-logging.jar on the server classpath, while Tomcat standalone use commons-logging-api.jar. I'm about to take on a project related to JBoss/Tomcat integration, so that will give me a chance to check it out more (i.e. stop filtering JCL in WEB-INF/lib and see what breaks). Had a chance to check this out, and the problem is because JBoss/Tomcat uses commons-logging.jar instead of the commons-logging-api.jar used by standalone Tomcat. When a webapp is deployed JBoss also deploys container code that executes when the webapps classloader is the TCCL (e.g. a valve that helps support the JACC spec). This code fails with LogConfigurationExceptions due to incompatible Log interfaces. JBoss can't just use commons-logging-api.jar as they use log4j for server logging. Good news is, the version of JCL in svn fixes this problem :) Well, not quite -- it fixes it once a patch is applied (see bugzilla 35423). But, they'll probably still keep filtering JCL once a new version of JCL is out in order to keep apps still using old versions from blowing up. BTW, in their nightly builds they've started using a patched version of JCL whose LogFactory uses a WeakHashMap instead of a Hashtable. This fixes memory leak problems in their unit tests. __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] proposed design for 1.0.5
(resuming an old thread... see http://marc.theaimsgroup.com/?l=jakarta-commons-devm=111688890907693w=2 for the history) --- Simon Kitching [EMAIL PROTECTED] wrote: On Mon, 2005-05-23 at 21:27 +0100, robert burrell snip donkin wrote: (e) That we remove the weakref stuff that was added since 1.0.4 (sorry, Brian!). The problem is that having Log deployed via the parent loader and subclasses of Log (eg Log4JLogger) deployed via the child classloader creates exactly the situation that renders the weakref stuff useless. Instead, I would just document clearly that people should use a ServletContextListener in situations where memory leaks matter. As I describe below, I don't think it's all that often. We could also provide cut-and-paste code to help them create and configure the ServletContextListener - or maybe even bundle a suitable implementation in the commons-logging.jar file. i agree that ServletContextListener is the preferred solution for web containers. i'd support an implementation shipping in the optional jar. however, i do think that there are configurations for other containers where the weak reference stuff may prevent or reduce memory leaks. JCL has been widely (and rightly) criticised for leaking memory in situations where this could have been avoided by using weak references. we have code that addresses these criticisms. i think we should ship it. I actually believe that JCL has been criticised for leaking memory in situations where *people who haven't analysed the problem properly believe weak references would solve the issue*. I don't believe that weakrefs *will* solve the problem in those situations. But I don't think the weakref stuff does any harm either (as long as there are no bugs in it!). So I'd be -0 to including it, not -1. If the WeakHashtable stuff is going to stay, I'm wondering if it should be in the main jar. If it's in the optional jar we have to explain how to use it, which could easily be interpreted as implying it will solve memory leaks, which in many cases it won't. If we at the same time encourage using commons-logging-adapters.jar (which will defeat the WeakHashtable), well, let's just say I'll be looking forward to the Bile Blog entry :) The downside to having it in the main jar is it will be there by default, adding runtime overhead. I'm personally indifferent on this (even as to whether it stays at all), but thought the above was worth pointing out. Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: DO NOT REPLY [Bug 34661] - [logging][PATCH] Improvements to LogFactoryImpl
--- Simon Kitching [EMAIL PROTECTED] wrote: On Tue, 2005-06-14 at 23:31 -0700, Brian Stansberry wrote: Re the InvocationTargetException for LogKitLogger: Does it actually matter whether ExceptionInInitializerError or InvocationTargetException occurs for LogKitLogger? [snip] Depends on how we resolve the issue of whether to ever fall into discovery if a relevant ALLOW...PROPERTY was set. In the case of InvocationTargetException, ALLOW_FLAWED_DISCOVERY_PROPERTY would come into play. True. As I said in the bugzilla note, I'm tempted to leave things as they are (no attempt to recover from problems when user specifies a particular library themselves). But I'm open to persuasion... Not going to try to persuade, as I've come around to your point of view. My nagging concerns were 1) avalon/logkit users would not be able to get JCL to run in certain situations and 2) the different behavior when using commons-logging.properties would be confusing to document and explain. Had a chance to look carefully at all the places where behavior with c-l.properties differs from normal discovery. As you've noted, many could be resolved with standard advice like use the commons-logging-adapters.jar or put a copy of logkit.jar in the parent loader classpath. The rest were odd cases involving a broken TCCL where some kind of special instructions would have to be given in any case. A big upside of the no attempt to recover approach is even log4j users can use c-l.properties and thus force LCE's in situations where JCL would otherwise fall back to JDK logging. For example, to avoid a situation where someone forgets to deploy log4j.jar in the parent classpath, so the special wake up the SA at 4:00AM whenever there is an error appender never gets called. Even if we decide _not_ to ever fall into discovery, its mildly tempting to patch LogKitLogger so it fails with the ExceptionInInitializerError. Just so if some poor guys three years from now change something, the bug won't suddenly pop out. That's fine by me. If you provide a patch I'll happily commit it. Will do. Best, Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: DO NOT REPLY [Bug 34661] - [logging][PATCH] Improvements to LogFactoryImpl
... shifting discussion to dev list from bugzilla entry http://issues.apache.org/bugzilla/show_bug.cgi?id=34661 --- Additional Comments From [EMAIL PROTECTED] 2005-06-14 13:20 --- Re the InvocationTargetException for LogKitLogger: Does it actually matter whether ExceptionInInitializerError or InvocationTargetException occurs for LogKitLogger? It does for logadapters that are part of auto-discovery as we want to continue discovery in the former case but not the latter. But LogKitLogger is not part of auto-discovery, and obviously no out-of-tree logger will be. So the only way LogFactoryImpl will ever try these loggers is when specificLogClassName != null - but in that case, we throw an LCE regardless of whether ExceptionInInitializerError or InvocationTargetException occurred, yes? Depends on how we resolve the issue of whether to ever fall into discovery if a relevant ALLOW...PROPERTY was set. In the case of InvocationTargetException, ALLOW_FLAWED_DISCOVERY_PROPERTY would come into play. And all the auto-discovered loggers should now be working as expected. So I think (hope) things are ok as they are.. Even if we decide _not_ to ever fall into discovery, its mildly tempting to patch LogKitLogger so it fails with the ExceptionInInitializerError. Just so if some poor guys three years from now change something, the bug won't suddenly pop out. Re comment #33 point 3: Yep, I pretty much agree with your analysis. However there is another option: just choose not to support this, and make the users fix their problem properly. Most of the problems reported against JCL are by users who are trying to use JCL out of the box and are confused it doesn't work. And I sympathise with them - they don't *want* to use JCL, it just came with a library and they just want it to shut up and not interfere with them running their app. I think people who are explicitly specifying logadapters via commons-logging.properties files are generally likely to be able to fix their classpath problems. And the next release of JCL should include a separate adapters-only jar which is the *correct* solution to this problem. So I'm inclined to just ignore this problem. People who specify a log class will be no worse off than JCL 1.0.4 if they throw commons-logging.jar in their webapp, and won't have any problems at all if they use commons-logging-adapters.jar (or whatever we choose to call it). Comments? What you're saying makes a lot of sense. But right now my tired brain is refusing to focus, so.. snip PS: thanks a lot for your comments/discussion of logging. It's really making me think :-). For such a small project this really is quite difficult isn't it?! It's really quite amazing. I kind of drifted into this, but the intellectual challenge of kind of got me hooked. Thanks much for putting so much effort into this, it's making it quite enjoyable and I think the end result will be worth it. Brian __ Discover Yahoo! Find restaurants, movies, travel and more fun for the weekend. Check it out! http://discover.yahoo.com/weekend.html - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] branches/allow-flawed
--- Simon Kitching [EMAIL PROTECTED] wrote: Hi, As you may have seen from the commit email, I have created a branch allow-flawed. This contains an attempt to implement fallback as described in this bugzilla entry: http://issues.apache.org/bugzilla/show_bug.cgi?id=34661 This looks quite nice. The flexibility that comes with the 3 different ALLOW_FLAWED... properties is interesting. For example (just thinking out loud here), a default of false for ALLOW_FLAWED_CONTEXT and ALLOW_FLAWED_DISCOVERY might make sense. These really indicate something strange, vs. the more normal hierarchy problem. The code in this branch passes all unit tests, and 100% of the demonstration tests. It does have one TODO item in there to do with AccessController but that's not relevant for the moment. Was going to ask about the missing AccessController stuff, but then reread your note :) Best regards, Brian __ Discover Yahoo! Get on-the-go sports scores, stock quotes, news and more. Check it out! http://discover.yahoo.com/mobile.html - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: DO NOT REPLY [Bug 34661] - [logging][PATCH] Improvements to LogFactoryImpl
snip from: http://issues.apache.org/bugzilla/show_bug.cgi?id=34661 --- Additional Comments From [EMAIL PROTECTED] 2005-05-31 07:49 --- I still don't understand that bit with the else around the log4j discovery. If someone has specified an explicit logclass, ie if (specifiedLogClassName != null) is true then surely: * we create an instance of the specified class and return it, or * we fail with an exception I don't think we should *ever* fall back to discovery mode if specifiedLogClassName is not null. I agree with this philosophy but wanted to point out that if a user both: 1) configures a Log class using a property, and 2) uses a property to configure JLC NOT to fail on an error throwing an exception will contravene #2. I don't have a problem with this, it just means the explanation of the usage of the FailOnException configuration property gets a bit more complicated. BTW, it would be somewhat strange for a user to configure the FailOnException property to false, since that is its default value anyway. I'm raising this point now because in the next day or two I'll create the next patch, the one that adds the configuration ability. If anyone has any opinions on this point (Robert??) please let me know. Best, Brian __ Do you Yahoo!? Make Yahoo! your home page http://www.yahoo.com/r/hs - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] LoadTest
--- Simon Kitching [EMAIL PROTECTED] wrote: On Tue, 2005-05-31 at 17:59 +1200, Simon Kitching wrote: The test doesn't set up its own clean classpath?! I thought that was what the AppClassLoader private class was all about - though of course it's hard to tell as there aren't any comments on it. It's just used to create the child-first child classloader -- doesn't affect the parent classpath. I think its trying to do a similar thing as the ClassReloader in beanutils. If the unit test doesn't properly set up a clean classpath before doing classpath-related unit tests, then that really sucks. Ah well, I don't think any of us have enough enthusiasm to fix the unit tests as well as the code. So however you get the test to run is ok by me. Hmm. I just tried your patch to include LoadTest in TestAll. And the test LoadTest fails for me with some problem in the reload method. That's without having applied any of your patches, ie the current HEAD version of the test fails against the current HEAD version of JCL. Strange. I ran it against HEAD yesterday and it worked, and just double checked and it still works. I suspect the reason the test isn't in the standard suite is that it has never actually worked properly at all. The custom ClassLoader user probably doesn't properly isolate the test from the surrounding environment meaning that the test will work for some people but not others (eg you, but not me). The subversion history shows that while license text etc. have been updated in it, no-one has actually committed a *meaningful* change to it since it was created at 2003-03-01. I might try to find time to have a look at this, but as the test was never part of the standard suite, I'm now generally in favour of just ignoring it. Haven't looked carefully, but the tests using Wrapper probably cover a lot of the same ground. Brian __ Discover Yahoo! Find restaurants, movies, travel and more fun for the weekend. Check it out! http://discover.yahoo.com/weekend.html - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] LoadTest
--- robert burrell donkin [EMAIL PROTECTED] wrote: 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. Your new diagnostics are really helpful here. They show exactly what happens. First, by the design of the test JCL has to be visible to parent classloader, which is the test case's classloader. This is because the child-first loader the test creates actually uses the parent classloader to find the class file. When I run this either using ant or Eclipse, the system classloader and the test case's loader are the same (i.e. the testing framework does not add a layer). LogFactory and LogFactoryImpl are loaded by the child loader. TCCL is set to the parent/system. Discovery analyzes the parent classloader. If log4j.jar is present, it discovers log4j. If not, it discovers jdk14. Discovery fails because Log interface implemented by discovered adapter is defined by the parent, while LogFactoryImpl is defined by the child. snip 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. +1 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 With the new code (aka rev 3b), if log4j.jar is visible to the parent classloader it will be discovered. Assume the scenario above; log4j.jar is visible tot the parent so discovery using the bogus TCCL finds a binary incompatible Log4jLogger. This error will be caught, and an attempt will be made to load Log4jLogger using the LogFactoryImpl's loader (the child). This will succeed, because log4j.jar can be loaded via parent delegation. I'm not sure if this is a flaw (maybe the child really wanted jdk logging, and we're giving them log4j), but basically we're discovering the log method made available via the TCCL. In any case, I don't think we can test for jdk or SimpleLog being discovered; we can only test if logging worked. Brian __ Do you Yahoo!? Yahoo! Mail - You care about security. So do we. http://promotions.yahoo.com/new_mail - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] LoadTest
--- Simon Kitching [EMAIL PROTECTED] wrote:
On Mon, 2005-05-30 at 21:22 -0700, Brian Stansberry
wrote:
I'm not sure if this is a flaw (maybe the child
really
wanted jdk logging, and we're giving them log4j),
but
basically we're discovering the log method made
available via the TCCL.
I think that if log4j *can* be used, and we find it,
then that is right.
Autodiscovery is meant to find log4j before jdk14.
And I'm not too bothered by discovering the wrong
logging
implementation anyway; users can always use a
commons-logging.properties
or a META-INF/services file to specify which logging
lib they want if
they really care.
What's important is that the discovered logging lib
*works* (doesn't
throw an exception).
+1
In any case, I don't think we can test for jdk or
SimpleLog being discovered; we can only test if
logging worked.
Isn't it possible for the test to do this?
if (running on jdk= 1.4) {
assertEquals(logObject.getClass(),
org.apache.commons.logging.impl.Jdk14Log);
} else {
assertEquals(logObject.getClass(),
org.apache.commons.logging.impl.SimpleLog);
}
After all, that's stating what we expect - that in
the setup used by
this test, we get logger Jdk14 or SimpleLog, and not
Log4JLog. The only
tricky bit is that the unit test currently doesn't
have access to the
Log object created, so it may mean a bit of
refactoring of the test
code.
Sorry, wasn't clear about what I meant. The test
classpath used in the ant build includes Log4j, so it
will be discovered (unless we create a special
classpath for this one test). In any case, whether or
not a particular adapter is discovered depends on
what's on the test classpath. We could have the test
analyze its classpath in order to determine what
should have been discovered, so I guess we *could*
test to see if the right one was discovered.
Brian
Regards,
Simon
-
To unsubscribe, e-mail:
[EMAIL PROTECTED]
For additional commands, e-mail:
[EMAIL PROTECTED]
__
Do you Yahoo!?
Yahoo! Small Business - Try our new Resources site
http://smallbusiness.yahoo.com/resources/
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] LoadTest
snip --- Simon Kitching [EMAIL PROTECTED] wrote: Brian: my comment re copy/paste error in comments for Lumberjack test refer to this line: // Other throwables just mean couldn't load the adapter or j.u.l in method discoverLogImplementation. I think the or j.u.l shouldn't be there? Jdk13LumberjackLogger imports java.util.logging :) __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
[logging] LoadTest
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. 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. Any thoughts on this one? Have I missed something? BTW, LoadTest is not invoked by the ant test target. Regards, Brian __ Do you Yahoo!? Yahoo! Small Business - Try our new Resources site http://smallbusiness.yahoo.com/resources/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
[logging] Exception handling in LogFactoryImpl [WAS] a candidate explanation for Log4JLogger does not implement Log
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Sun, 2005-05-22 at 15:59 -0700, Brian Stansberry wrote: --- robert burrell donkin [EMAIL PROTECTED] wrote: On Sun, 2005-05-08 at 01:06 +1200, Simon Kitching wrote: Hi All, I've been working for quite a while now to try to figure out why users of JCL have been getting the Log4JLogger does not implement Log message. I think I've finally really understood the cause. If this is really obvious to everyone, please let me down gently :-) one thing which i have learnt from working with JCL over the years is that nothing is ever obvious :) more than anything we need clear explanations of what happens and why. +1. I understood (and had since forgotten) from working with Ceki Gulcu's JCL analysis that the problems happened when code loaded a parent loaded tried to log with the TCCL in effect, but your discussion of why such a call would be made -- outside a test fixture :) -- makes the real world issue much clearer (and easier to remember). snip Ok, so what is the solution? i would like to split this question into two. i believe (as indicated in the analysis document) that the correct behaviour in these cases is for JCL to recognise that log4j is not viable (for this configuration) and default to java.util logging. this is a little unsatisfactory but i don't see a reasonable technical solution for these configuration. One unsatisfactory aspect is that instead of throwing an exception with a nice message and stack trace, logging would mysteriously be done using an unexpected logging library. But Simon's recent work on adding diagnostics should help mitigate this drawback. i think that this comes down to a question of philosophy. the consequences of throwing an exception are usually pretty fatal for an application. personally, i think that we'd be doing most users (who just want to run their application) a favour by ensuring that discovery throws as few exceptions as possible. i hope that diagnostics and a good troubleshooting document would prove a good enough substitute for those who want to choose a particular logging system. then again, this opinion has traditionally been in the minority so i'd be happy to go with the consensus on this issue... I've had a chance to look at the refactored version of LogFactoryImpl I'd done, and it should be quite simple to offer configurable options for this. The current patch includes a method recordCreationFailure that gets called when an adapter is discovered but fails in the instance creation process. It's simple to rename that method to something more general and add a bit of logic to check a configuration option (i.e system property or a property in commons-logging.properties), and either throw a LogConfigurationException or record the exception using the new diagnostics code. Which way to go as a default when the config option isn't set is another question. Personally, I'm also of the logging should not fail an app opinion. It should also be fairly simple to cache any exception that occurs during the discovery process, and then if a Log is eventually found to record the cached exception as a warning. This will provide feedback to users who don't have the JCL diagnostics turned on. Best, Brian - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Yahoo! Mail Mobile Take Yahoo! Mail with you! Check email on your mobile phone. http://mobile.yahoo.com/learn/mail - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] detecting logging libs
--- robert burrell donkin
[EMAIL PROTECTED] wrote:
On Mon, 2005-05-23 at 11:24 +1200, Simon Kitching
wrote:
On Sun, 2005-05-22 at 21:01 +0100, robert burrell
donkin wrote:
On Sun, 2005-05-22 at 10:43 +,
[EMAIL PROTECTED] wrote:
snip
protected boolean
isJdk13LumberjackAvailable() {
+// note: the algorithm here is
different from isLog4JAvailable.
+// I think isLog4JAvailable is
correctsee bugzilla#31597
+1
could do with going through all the
isAvailable's and checking whether
their algorithms are correct. however, i suspect
that brian's approach
will be needed to deal correctly with some
circumstances. if no one
feels like volunteering should probably record
this in bugzilla so we
don't lose track...
If by brian's approach you mean creating an
instance of the logger
class in order to test whether that logging lib is
*really* available, I
agree.
yes
(hopefully brian will jump in here and correct any
misunderstandings)
+1
Not only is it more reliable, but it's a cleaner
solution; currently the
LogFactoryImpl class is making *assumptions* about
what classes the
various logging adapters depend on. That
information should be only in
the logging adapter class.
+1
in addition, the specification allows variation as
to the timing of
error reporting. i believe that creating an
instances would give more
consistency across JVMs.
The only problem with creating an instance of the
logger is that we
would have to pass a category string to the logger
constructor, and
therefore must build an assumption into
LogFactoryImpl about what
category names are valid for the underlying
logger. Can we assume that
an empty string is a valid category for all logger
libraries? Can we
assume that apache or
org.apache.commons.logging are valid category
strings? Perhaps some loggers only accept valid
URLs as
categories...yes, I'm playing devil's advocate a
bit here. I guess we
could always say that the writer of the logging
adapter is required to
return a valid logger instance for category ,
even if that is not
normally a category that is valid to the
underlying library.
IIRC brian's patch refactored the code so that the
test also constructed
the correct logger instance (or something like
that). if you don't beat
me to it, i'll commit the patch onto one of the
branches so that
everyone can easily take a look at the approach.
(again hopefully brian will jump in here if i've
made any mistakes)
Looks good.
- robert
-
To unsubscribe, e-mail:
[EMAIL PROTECTED]
For additional commands, e-mail:
[EMAIL PROTECTED]
__
Do you Yahoo!?
Yahoo! Small Business - Try our new Resources site
http://smallbusiness.yahoo.com/resources/
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] proposed design for 1.0.5
--- Simon Kitching [EMAIL PROTECTED] wrote: On Mon, 2005-05-23 at 21:27 +0100, robert burrell donkin wrote: (b) That in addition to commons-logging.jar we distribute a commons-logging-adapters.jar which contains the entire contents of the impl subdirectory, ie all the logging adapters and the concrete LogFactory subclasses, but specifically NOT * LogFactory * Log [this is along the lines of Brian's jar-refactoring proposal] We should do away with commons-logging-api.jar; it serves no purpose. -1 a few reasons: 1. it is vital for dependency management. it is very important that JCL ships a dependency free jar for compilation and dependency management. the main flaw with the api is that it's too big and not viable by itself. Sorry, I don't understand. Why can't people compile against commons-logging.jar? 2. it is useful for certain parent first configurations. replacing an full jar with an api jar allows some parent first configurations to log to log4j. I don't see why this would be true. What configurations does an api jar support that a full commons-logging.jar won't? I believe it allows a child to use commons-logging.properties to specify a logging library that is not visible to the parent. With only the -api jar in the parent, the Log adapter will be defined by the child and will thus be compatible with the library. See examples 10 and 10.i in http://xnet.wanconcepts.com/jcl/furtherAnalysis.html 3. backwards compatibility. (would need a 1.1 release at least.) I'm happy with making the next release 1.1. It should be totally backwards-compliant anyway. (c) That we change the error message when multiple Log instances found, to state that child should contain -adapters.jar not full jar. +1 we need to be confident about the diagnostics in this case before recommended definite action. (i believe that some exotic classloader configurations may also produce similar symptoms) i certain think that the wording should be improved but would welcome a reference to troubleshooting document (possibly an url?) which could provide more explanation and could offer advice for the more exotic cases. Yep, it's hard to be confident we've understood the situation completely until we get feedback from people in the field. But the fact that they can turn on diagnostics and email us the output should improve that situation greatly. As you say, we could be a bit careful about the wording of the error message, just in case it turns out that there are some other situations that trigger the same message...eg This is probably caused by ... (d) That we provide the deployment instructions listed at the bottom of this email. in some ways discovery deployment instructions are both needed and a contradiction. discovery is supposed to guess right but sometimes it needs a little help. JCL needs a troubleshooting document. (i even made a start on one a while ago.) the deployment instructions/recommendations would fit very, very into such a document. i like having information on the web (rather than in release notes etc). not only is it easy to reference when answering user questions but it's also easily indexed by search engines. if this sounds like a good plan, i'll try to pull something along those lines together in the next few days. +1 (e) That we remove the weakref stuff that was added since 1.0.4 (sorry, Brian!). The problem is that having Log deployed via the parent loader and subclasses of Log (eg Log4JLogger) deployed via the child classloader creates exactly the situation that renders the weakref stuff useless. Instead, I would just document clearly that people should use a ServletContextListener in situations where memory leaks matter. As I describe below, I don't think it's all that often. We could also provide cut-and-paste code to help them create and configure the ServletContextListener - or maybe even bundle a suitable implementation in the commons-logging.jar file. i agree that ServletContextListener is the preferred solution for web containers. i'd support an implementation shipping in the optional jar. however, i do think that there are configurations for other containers where the weak reference stuff may prevent or reduce memory leaks. JCL has been widely (and rightly) criticised for leaking memory in situations where this could have been avoided by using weak references. we have code that addresses these criticisms. i think we should ship it. I actually believe that JCL has been criticised for leaking memory in situations where *people who haven't analysed the problem properly believe weak references would solve the issue*. I don't believe that weakrefs *will* solve the problem in those situations.
Re: [logging] detecting logging libs
--- Simon Kitching [EMAIL PROTECTED] wrote: On Sun, 2005-05-22 at 20:56 -0700, Brian Stansberry wrote: --- Simon Kitching [EMAIL PROTECTED] wrote: I'd be happy with including this approach in the next release, removing the redundant isXYZAvailable methods and calling the next release 1.1. In practice, I'm sure no-one *does* subclass LogFactoryImpl so there's no problem. In fact I'd be happier with this than a 1.0.5 where the methods exist but don't do anything. +1 Ok. So questions: * Robert are you ok with adopting Brian's approach to checking a logger is ok (by creating one) rather than using the current isXYZAvailable? * Robert, are you ok with ripping out the isXYZAvailable methods and calling the next release 1.1? * Brian: if Robert's ok with this (and no-one else votes -1), will you prepare an actual patch or should I? I'd be happy to. By the way, Brian, are you an Apache committer? No. Regards, Simon - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Yahoo! Mail Mobile Take Yahoo! Mail with you! Check email on your mobile phone. http://mobile.yahoo.com/learn/mail - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] a candidate explanation for Log4JLogger does not implement Log
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Sun, 2005-05-08 at 01:06 +1200, Simon Kitching wrote: Hi All, I've been working for quite a while now to try to figure out why users of JCL have been getting the Log4JLogger does not implement Log message. I think I've finally really understood the cause. If this is really obvious to everyone, please let me down gently :-) one thing which i have learnt from working with JCL over the years is that nothing is ever obvious :) more than anything we need clear explanations of what happens and why. +1. I understood (and had since forgotten) from working with Ceki Gulcu's JCL analysis that the problems happened when code loaded a parent loaded tried to log with the TCCL in effect, but your discussion of why such a call would be made -- outside a test fixture :) -- makes the real world issue much clearer (and easier to remember). snip Ok, so what is the solution? i would like to split this question into two. i believe (as indicated in the analysis document) that the correct behaviour in these cases is for JCL to recognise that log4j is not viable (for this configuration) and default to java.util logging. this is a little unsatisfactory but i don't see a reasonable technical solution for these configuration. One unsatisfactory aspect is that instead of throwing an exception with a nice message and stack trace, logging would mysteriously be done using an unexpected logging library. But Simon's recent work on adding diagnostics should help mitigate this drawback. the other question (which i think is what simon addresses below) is how can JCL provide a solution for a user who needs a similar configuration but who is willing to choose to deploy JCL slightly differently. (1) Well, ensuring that the logging adapters are deployed via the parent classloader *only* is one. That fixes the problem, as Log4JLogger et. al. always bind to the same Log interface that LogFactory binds to. The downside is: * It means that the logging library must also be deployed via the parent classloader, even when there *are* no other classes in the parent classloader that trigger this problem (ie ones that use JCL and that the webapp is going to call into). Having the logging library loaded via the parent classloader means that dumb logging libraries may not be able to find their config files. Some logging libs may be smart enough to look for their resource files via the context classloader. And in some cases the logging *adapter* class might be able to tell the logging lib where the config file is. Assuming we can rely on (or trick) the logging lib to correctly handle per-context-classloader location of their config files, all should be well. -1 seems like giving away too much for a corner case (2) I think we could simply change the distribution bundles. The root problem appears to be to me that the adapters (Log4JLogger et al) are bundled with a Log implementation. If we produced a jar that included Log4JLogger et al. but *without* the Log class, the problem should be solved. The rule would then be: * if the parent loader has commons-logging.jar or commons-logging-api.jar, then deploy commons-logging-adapters.jar in the child, together with the actual logging library. * otherwise, deploy commons-logging.jar in the child (or commons-logging-api.jar + commons-logging-adapters.jar) this approach to these kinds of configuration is the one i was thinking of earlier when i suggested that we might need an implementations only distribution. a reasonable user could then adjust the deployment so that the implementations were in the child and the api in the parent. (3) Maybe we should just do away with LogFactoryImpl's attemp to load a log adapter via the context classloader. It does enable the setup of having commons-logging-api.jar in the parent and commons-logging.jar in the child, but it fails badly if any other class in the parent classloader uses JCL and is called by the webapp. Is this benefit worth the pain? no but there is a variation (that i have discussed with richard previously). the particular problem situation can be diagnosed (a log class loaded from the context should have an incompatible classloader). when the implementation is not viable, rather than throw an exception JCL could try to load the class with the same name from it's classloader. i think that brian's patch does something which though implemented differently has the same net result. however, i don't think that either of these approaches would actually work in this case: log4j is not visible to the parent classloader. i do think that something along these lines will be needed for the exotic classloader configurations (which haven't been analysed yet). BTW, Brian proposed a splitting-up of the JCL jars a few months ago, as experiments showed it
Re: [logging] proposed design for 1.0.5
--- Simon Kitching [EMAIL PROTECTED] wrote: Hi, = I would like to propose the following for jcl 1.0.5: snip (e) That we remove the weakref stuff that was added since 1.0.4 (sorry, Brian!). No problem -- I realize as I write this that I've been subconsciously waiting for the ax to fall ever since you posted about the memory leak problem in beanutils ;) Teaches me (yet again) not to try to solve a problem without fully understanding the problem domain. __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] detecting logging libs
--- Simon Kitching [EMAIL PROTECTED] wrote:
On Sun, 2005-05-22 at 21:01 +0100, robert burrell
donkin wrote:
On Sun, 2005-05-22 at 10:43 +,
[EMAIL PROTECTED] wrote:
snip
protected boolean
isJdk13LumberjackAvailable() {
+// note: the algorithm here is
different from isLog4JAvailable.
+// I think isLog4JAvailable is
correctsee bugzilla#31597
+1
could do with going through all the isAvailable's
and checking whether
their algorithms are correct. however, i suspect
that brian's approach
will be needed to deal correctly with some
circumstances. if no one
feels like volunteering should probably record
this in bugzilla so we
don't lose track...
If by brian's approach you mean creating an
instance of the logger
class in order to test whether that logging lib is
*really* available, I
agree.
Not only is it more reliable, but it's a cleaner
solution; currently the
LogFactoryImpl class is making *assumptions* about
what classes the
various logging adapters depend on. That information
should be only in
the logging adapter class.
The only problem with creating an instance of the
logger is that we
would have to pass a category string to the logger
constructor, and
therefore must build an assumption into
LogFactoryImpl about what
category names are valid for the underlying logger.
Can we assume that
an empty string is a valid category for all logger
libraries? Can we
assume that apache or org.apache.commons.logging
are valid category
strings? Perhaps some loggers only accept valid URLs
as
categories...yes, I'm playing devil's advocate a bit
here.
It's a good point; the assumed category names in the
various isXYZAvailable method is my biggest dislike
about the patch I submitted. Actually, if an approach
like the patch were adopted, I think we should
consider marking those methods as deprecated (they're
not used in the patched LogFactoryImpl itself) and
dropping them at the next release with a point scope
that allows it.
In general, if any significant work on LogFactoryImpl
is done I think a review of its API would be good.
This seems to me to be the kind of class that if
someone wants a different implementation, they should
write a different implementation and not count on a
lot of behaviors exposed by a superclass.
Of course, any kind of deprecation doesn't solve the
immediate problem.
Looking into it more, I remembered that
LogFactoryImpl.getInstance(Class clazz) just calls
getInstance(clazz.getName()) and thus already creates
a requirement that Log implementations support fully
qualified class names. So any adapter that doesn't do
this must be designed to work with a custom
LogFactory. So, the potential list of users affected
by this issue is smaller -- only those with custom
LogFactory implementations and Log implementations
that don't support class names.
I guess we
could always say that the writer of the logging
adapter is required to
return a valid logger instance for category , even
if that is not
normally a category that is valid to the underlying
library.
Half-baked idea to throw out before I run. Add to
LogFactoryImpl:
/**
* Returns a category that can be used to test whether
* an instance of a codeLog/code can be created.
* p
* This default implementation returns the fully
* qualified name of this class. Subclasses should
* override this if they support codeLog/code
* implementations that cannot handle this category.
* /p
*/
protected String getTestCategory() {
return getClass().getName();
}
All the various isXYZAvailable methods would call this
method when creating a test instance.
Brian
Regards,
Simon
-
To unsubscribe, e-mail:
[EMAIL PROTECTED]
For additional commands, e-mail:
[EMAIL PROTECTED]
__
Yahoo! Mail Mobile
Take Yahoo! Mail with you! Check email on your mobile phone.
http://mobile.yahoo.com/learn/mail
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] detecting logging libs
--- Simon Kitching [EMAIL PROTECTED] wrote:
On Sun, 2005-05-22 at 18:38 -0700, Brian Stansberry
wrote:
It's a good point; the assumed category names in
the
various isXYZAvailable method is my biggest
dislike
about the patch I submitted. Actually, if an
approach
like the patch were adopted, I think we should
consider marking those methods as deprecated
(they're
not used in the patched LogFactoryImpl itself) and
dropping them at the next release with a point
scope
that allows it.
Ah - I see. With your approach, you just pass the
name that the user
provided, so don't need a dummy category name at
all. Fine.
Regarding deprecation, if we're going to change the
logic flow within
the LogFactoryImpl such that the existing
isXYZAvailable methods are no
longer called, then to me that's *just as much* an
incompatible change
as removing the methods.
Removing the methods potentially causes subclasses
to not compile/link,
but leaving them there and not calling them causes
subclasses to
*logically* fail, which in my view is even worse
because it's not
visible.
+1
I'd tended to think about this in terms of a subclass
writing its own logic flow but calling into the
various isXYZAvailable methods, but you're right, a
subclass could also count on the superclass logic flow
but override an isXYZAvailable. This is more likely
even.
I'd be happy with including this approach in the
next release, removing
the redundant isXYZAvailable methods and calling the
next release 1.1.
In practice, I'm sure no-one *does* subclass
LogFactoryImpl so there's
no problem. In fact I'd be happier with this than a
1.0.5 where the
methods exist but don't do anything.
+1
A major refactor of the core class in the library
seems a bit much for two-decima point release name.
In general, if any significant work on
LogFactoryImpl
is done I think a review of its API would be good.
This seems to me to be the kind of class that if
someone wants a different implementation, they
should
write a different implementation and not count on
a
lot of behaviors exposed by a superclass.
I'm working on a major refactoring proposal now,
that will not just
review but gut LogFactoryImpl :-).
Looking into it more, I remembered that
LogFactoryImpl.getInstance(Class clazz) just calls
getInstance(clazz.getName()) and thus already
creates
a requirement that Log implementations support
fully
qualified class names. So any adapter that
doesn't do
this must be designed to work with a custom
LogFactory. So, the potential list of users
affected
by this issue is smaller -- only those with custom
LogFactory implementations and Log implementations
that don't support class names.
Yes, good point. We should really document that in
the Log interface. In
order to be able to swap in/out alternative logging
implementations,
there does need to be a standard for category
names that the concrete
Log classes can all handle. It's implicit in the
whole concept of JCL
(well, any logging wrapper).
Half-baked idea to throw out before I run. Add to
LogFactoryImpl:
/**
* Returns a category that can be used to test
whether
* an instance of a codeLog/code can be
created.
* p
* This default implementation returns the fully
* qualified name of this class. Subclasses
should
* override this if they support codeLog/code
* implementations that cannot handle this
category.
* /p
*/
protected String getTestCategory() {
return getClass().getName();
}
All the various isXYZAvailable methods would call
this
method when creating a test instance.
No need I think. Your code in bugzilla which simply
uses the first log
category passed in by the user seems fine to me.
Good. The above would only be relevant if the unused
isXYZAvailable methods were left in, and you make a
good argument for removing them.
One minor issue: if the user passes in an invalid
category name of
some sort, then we might assume that the logging
implementation is not
available. Not a likely problem though.
And as you point out above, JCL effectively imposes
its own standard on
what is and is not a valid category name anyway, so
if the above is
really considered a problem then we can just declare
that is always a
valid category name that returns the root
category, and all Log
subclasses need to handle that category name.
Hmm.. what about when a logging implementation is
available, but
attempting to create a logger fails, eg because the
underlying
implementation's config file is bad. If we test for
the existence of the
logging lib by creating a logger, then we would fall
back to other
logging libs in this case - is that what we want?
I think this comes down to the general philisophical
question -- should a logging framework be able to fail
an application? There's good arguments on both sides
which has got me
Re: [logging] requirements and static binding
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Thu, 2005-05-05 at 23:12 +1200, Simon Kitching wrote: On Wed, 2005-05-04 at 23:37 -0700, Brian Stansberry wrote: snip 2) When checking into the above, I discovered that in the latest JBoss, their webapp classloader won't load commons-logging.jar from WEB-INF/lib even if parent-last loading is in effect. It's specifically disabled. Seems there were type conflicts with JCL classes loaded by the integrated Tomcat container. Not sure yet what this is all about, but in any case the net effect is that as far as JCL is concerned, a webapp on JBoss behaves as if parent-first loading were in effect. InterestingI wonder if they posted any questions to the JCL development list before adopting this (apparently fairly radical) solution. I'll go have a look. seems like an odd solution. any more information on this? Not too much. The JBoss CVS commit history and their JIRA don't give too many details. For JBoss 4.0.2 they switched to using the standard Tomcat webapp classloader by default (instead of their own). I'm *guessing* that the problem might arise because JBoss has commons-logging.jar on the server classpath, while Tomcat standalone use commons-logging-api.jar. I'm about to take on a project related to JBoss/Tomcat integration, so that will give me a chance to check it out more (i.e. stop filtering JCL in WEB-INF/lib and see what breaks). Sorry to have dropped out of sight -- wife and baby are heading home for a long period to see grandparents, so starting next week I'll be a bachelor programmer again. - Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] a candidate explanation for Log4JLogger does not implement Log
--- Simon Kitching [EMAIL PROTECTED] wrote: On Sun, 2005-05-08 at 01:06 +1200, Simon Kitching wrote: Ok, so what is the solution? (2) I think we could simply change the distribution bundles. The root problem appears to be to me that the adapters (Log4JLogger et al) are bundled with a Log implementation. If we produced a jar that included Log4JLogger et al. but *without* the Log class, the problem should be solved. The rule would then be: * if the parent loader has commons-logging.jar or commons-logging-api.jar, then deploy commons-logging-adapters.jar in the child, together with the actual logging library. * otherwise, deploy commons-logging.jar in the child (or commons-logging-api.jar + commons-logging-adapters.jar) Hmm..this approach might aggravate the memory leak on webapp reload issue. To recap, the LogFactoryImpl holds a map of (logger-name, log). If those log objects belong to a class loaded from the child classloader then they hold references to the child classloader. This prevents the LogFactory's (weak-ref-to-classloader, strong-ref-to-logFactoryImpl) map from freeing stuff related to a classloader, as there is a strong ref to the context-classloader via LogFactory-LogFactoryImpl-Logger-class-classloader. Recommending that commons-logging-api.jar be in the parent, but commons-logging-adapters seems to be exactly the scenario to trigger the unfixable-via-weakrefs leak :-(. I haven't proved this, and it's complicated enough that I might be wrong here. But I thought I should mention it now. Nope, sadly I'm quite sure you're right. Bug 31286 (http://issues.apache.org/bugzilla/show_bug.cgi?id=31286) has a unit test patch that simulates a parent has api, child has adapters classloading scenario. It fails with a memory leak. Haven't had a chance to digest your recent postings, so apologize for not giving more feedback. Brian Cheers, Simon - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] requirements and static binding
A few semi-random points on parent-first vs. parent-last classloading: 1) EJBs, EARs and other non-webapp J2EE deployments typically use parent-first loading. I'd thought JBoss offered a deployment descriptor option that allowed the deployer to choose parent-last, but I was mistaken. Too bad; I was hoping the scope of the if you want control, use parent-last classloading approach would apply. 2) When checking into the above, I discovered that in the latest JBoss, their webapp classloader won't load commons-logging.jar from WEB-INF/lib even if parent-last loading is in effect. It's specifically disabled. Seems there were type conflicts with JCL classes loaded by the integrated Tomcat container. Not sure yet what this is all about, but in any case the net effect is that as far as JCL is concerned, a webapp on JBoss behaves as if parent-first loading were in effect. 3) Thinking again of the DefaultServlet and JSPServlet in Tomcat. These classes are loaded by a container classloader, but the logging of a specific instance of the classes should be governed by the configuration of the webapp. AFAICT, this will require dynamic discovery based on the TCCL. I too prefer the simplicity (and lack of memory leaks) of static binding, but given the above and the recent discussion continue to see a long life for dynamic discovery as well. This gets me thinking of how carefully we're going to have to document things when we provide both static and dynamic discovery. For example, if we provide a commons-logging-jdk14.jar, we'll have to make clear that deploying it with your EJBs won't work if the container has commons-logging.log4j.jar on the classpath, that it won't work in a webapp on JBoss, etc. Brian __ Do you Yahoo!? Make Yahoo! your home page http://www.yahoo.com/r/hs - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] J2EE Spec and classloader order (WAS: requirements and static binding)
Branching this discussion off, as I realize my previous post forked a thread. --- Simon Kitching [EMAIL PROTECTED] wrote: On Wed, 2005-05-04 at 23:37 -0700, Brian Stansberry wrote: A few semi-random points on parent-first vs. parent-last classloading: 1) EJBs, EARs and other non-webapp J2EE deployments typically use parent-first loading. I'd thought JBoss offered a deployment descriptor option that allowed the deployer to choose parent-last, but I was mistaken. Too bad; I was hoping the scope of the if you want control, use parent-last classloading approach would apply. Well, if the EJB spec is designed to prevent EJBs from overriding jars present in the parent classloader, who are we to argue? Sorry, didn't cross-pollinate between a discussion I was having on the JBoss forum and here. I briefly looked at the J2EE and EJB specs and didn't see anything that *required* parent-first. It really doesn't say anything about it at all. A comment by Scott Stark at JBoss implies the same. For the JBoss forum discussion, please see: http://www.jboss.org/index.html?module=bbop=viewtopict=63520 In other words, if a JCL logging implementation is in the parent classloader, then why not just bind to it? This doesn't give the EJB developer any control over what logging lib is used (though they don't typically need such control). More controversially, it doesn't give any control to the application assembler, as any jar they bundle will be ignored if the container provides an implementation. But that's the way the J2EE spec wants things to work it appears. And potentially even more controversially, it doesn't give any control to the application deployer unless they are also a system administrator for the container (and are willing to change the logging lib globally). But again, if the J2EE spec authors chose parent first as the only option, then that must be what they wanted to happen. Or is it just JBoss that has adopted this position for EJB deployment I wonder? Could be (although my gut instinct says otherwise). Any BEA/Websphere/Geronimo/YourFavoriteAppServer experts out there know of support for child-first loading for EJBs? Brian __ Do you Yahoo!? Yahoo! Small Business - Try our new resources site! http://smallbusiness.yahoo.com/resources/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] issues highlighted by analysis
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Mon, 2005-04-25 at 13:53 -0700, Brian Stansberry wrote: --- robert burrell donkin [EMAIL PROTECTED] wrote: On Sun, 2005-04-24 at 16:43 -0700, Brian Stansberry wrote: --- robert burrell donkin [EMAIL PROTECTED] wrote: snip i'm wondering whether to commit them onto a branch so that everyone can take a look, check their accuracy and take a look at fixes. opinions? Please forgive if this is a stupid question, but why on a branch? to prevent a gump storm :) gump builds from TRUNK. committing unit tests that failure onto TRUNK means that gump will fail to build JCL. last time that happened, there were literally hundreds of dependent products that could not be built. each failed project means an email every day until it's fixed. thus, a gump storm. Wow. That's a shame. I'd think not being able to add unit tests that fail to the main line would tend to lead to a lot fewer unit tests. of course, they can be committed so long as they aren't run as part of the main test target. unit tests that are intended to fail would require some documentation and seems a bit strange for TRUNK. Thought about this more and now tend to agree that tests that fail are strange in TRUNK. The purpose of a unit test that fails is to document a problem even if no solution is apparent. But that's what a good bug tracking system is for; a good place to put tests that fail is as an attachment to a bug report. Once the problem is fixed, then the test can be moved into the codeline. given the general interest, i'll probably tidy up the test i have and commit them into TRUNK (for now) but i won't call the target from the main test target. BTW, a couple weeks back I added a unit test patch to the JBoss Memory Leak bug. The added test will fail, so the patch shouldn't be committed to trunk. (Thus confirming my point above). Or not ;) part of the required habit for a committer is to get in the right habits. once the work's finished and ready for committing, always update, build the distribution and run the standard unit tests. never commit a patch with broken unit tests. +1 i'm coming round to simon's idea that an additional directory (a peer to TRUNK and BRANCHES) may be the best solution. we could add memory issues analysis next to the discovery stuff. +1. BTW, My comment above re attaching to bug reports wasn't in reference to the new tests you've written. Brian - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do you Yahoo!? Yahoo! Small Business - Try our new resources site! http://smallbusiness.yahoo.com/resources/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] possible errors in child first cases
Sorry to waste your time; false alarm. The child-first cases are fine. Somehow my commons-logging.jar had everything but the kitchen sink in it, including the demonstration code. Good news is with that problem fixed and a refactored LogFactoryImpl, JCL worked as expected in all cases. Brian --- Brian Stansberry [EMAIL PROTECTED] wrote: --- robert burrell donkin [EMAIL PROTECTED] wrote: On Sun, 2005-04-24 at 23:59 -0700, Brian Stansberry wrote: --- robert burrell donkin snip When I ran these, for every child-first case the sysout output said the caller was defined by the child loader. [java] Callerdefined by CHILD class loader For 18, 20, 26, 28, 30 and 32, the overview says caller should be loaded by the parent. Haven't had a chance to try to see why, but this could be the cause of some problems. that's interesting. i've rerun the tests on my machine and all those tests seem to have the caller defined correctly. (i don't seem to have anything which isn't check in.) i'm running Java HotSpot(TM) Client VM (build 1.4.2_04-b05, mixed mode) on linux at the moment. anyone else experiencing these problems? Please note that the results I reported came when I ran the tests using the patched version of LogFactoryImpl I discussed. Don't know if that would matter or not. I can try again tonight using the normal JCL. Or it could be some other screwup on my part. Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] LogFactoryImpl weaknesses [WAS Re: [logging] issues highlighted by analysis]
--- robert burrell donkin [EMAIL PROTECTED] wrote: snip On Sun, 2005-04-24 at 23:59 -0700, Brian Stansberry wrote: I think a better approach would be more like the following: 1) newInstance checks if logConstructor is null. 2) if null, call a private Log discoverLogImplementation(String logName) method, passing the category name of the log. 3) discoverLogImplementation not only tries to find the class and c'tor, it actually instantiates the log and returns it. By doing this it can ensure that discovery actually works, including taking steps like the ones you recommend above. 4) If discoverLogImplementation succeeds in instantiating a Log, it sets instance variable logConstructor so in the future the newInstance method can instantiate logs directly. that sounds like a good plan. one thing to remember is that there is quite a lot of code in the various instantiation methods that has been added to cater for unusual environments over time. therefore modifying the control flow sounds like the right approach. Thanks for the heads up. After rearranging the contol flow, some of the existing checks *appear* unnecessary, but I need to be careful about subtleties. I roughed out an implementation along these lines tonight and got no exceptions on your demonstration tests. JCL worked as expected in all except 13, 14, 18, 20, 26, 28, 30 and 32. I think it was right in 13 and 14 as well, and am hoping there is a problem in the test fixture on the others. that's possible. (i'll discuss this in another thread.) Not a test fixture problem; more of a test user problem . Once the test user was adjusted, JCL worked as expected in all tests. once you're happy to left other people take a look at your code, add it to a bug report. I'll try to do that tomorrow or at most Wed. I won't be shy about posting early and having people find a few warts; I figure letting more eyes look at the subtleties is more important. Brian - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] issues highlighted by analysis
--- robert burrell donkin [EMAIL PROTECTED] wrote: here are my results obtained by comparing the theory with the current behaviour. (1) JCL Behaves As Expected (50%) - 1,2,3,4,7,8,11,12,15,16,17,18,19,23,27,31 (2) JCL Throws Exception Rather Than Logging To Log4J (28%) --- 5,6,13,14,21,22,24,25,29 On 14, shouldn't JCL logging be via j.u.l? The caller's classloader only has access to the api.jar, so it shouldn't be possible to load Lo4jLogger. On 13 a similar situation applies, because with parent-first loading the api.jar in the parent is going to be the JCL that does discovery. It will see its own Log4j but have no wrapper class. (3) JCL Throws Exception Rather Than Logging To java.util.logging (22%) --- 9,10,20,26,28,30,32 When I ran these, for every child-first case the sysout output said the caller was defined by the child loader. [java] Callerdefined by CHILD class loader For 18, 20, 26, 28, 30 and 32, the overview says caller should be loaded by the parent. Haven't had a chance to try to see why, but this could be the cause of some problems. i've categorised them by the differences between what should happen in theory with the current behaviour. i am going to create tests cases based on the ones in demonstration which should allow accurate diagnosis of the causes. however, i have a good idea of some of the causes and would like to start a discuss about possible solutions in advance. i don't propose to do anything about those in #1 ;) i suspect that many of failure cases in #3 are caused by JCL being able to load one or more of the appropriate classes by name from a classloader but then throw an exception when they are not compatible. IMHO this behaviour is counter-productive. in this case, rather than throwing a runtime exception, JCL should conclude that log4j is actually not really available and use java.util.logging instead. any opinions about changing JCL so that this approach is adopted? +1. If the chosen logger cannot be loaded, a message to System.out and/or System.err should be recorded, and then fall back to JDK logging, and then to SimpleLog. I don't think logging frameworks should throw runtime exceptions. i suspect that many of the failure cases in #2 are caused by JCL using an incompatible class from the context classloader when one is available from it's own classloader. IMHO the presence of a version of Log4J on the context classloader should be a good enough indication that the user intended log4j to be discovered for JCL to try to load log4j from it's own classloader if the other fails. opinions? Sounds right, but need to think on this some more. Don't know why but something tells me I should. I think your comments both point in the direction of LogFactoryImpl's biggest weakness. Currently discovery is really divided into 3 methods (newInstance, getLogConstructor and getLogClassName), arranged in a stack. The problem is if getLogClassName discovers a class the methods higher on the stack can't handle, they have no choice but to throw an exception. I think a better approach would be more like the following: 1) newInstance checks if logConstructor is null. 2) if null, call a private Log discoverLogImplementation(String logName) method, passing the category name of the log. 3) discoverLogImplementation not only tries to find the class and c'tor, it actually instantiates the log and returns it. By doing this it can ensure that discovery actually works, including taking steps like the ones you recommend above. 4) If discoverLogImplementation succeeds in instantiating a Log, it sets instance variable logConstructor so in the future the newInstance method can instantiate logs directly. I roughed out an implementation along these lines tonight and got no exceptions on your demonstration tests. JCL worked as expected in all except 13, 14, 18, 20, 26, 28, 30 and 32. I think it was right in 13 and 14 as well, and am hoping there is a problem in the test fixture on the others. Brian - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] issues highlighted by analysis
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Sun, 2005-04-24 at 16:43 -0700, Brian Stansberry wrote: --- robert burrell donkin [EMAIL PROTECTED] wrote: snip i'm wondering whether to commit them onto a branch so that everyone can take a look, check their accuracy and take a look at fixes. opinions? Please forgive if this is a stupid question, but why on a branch? to prevent a gump storm :) gump builds from TRUNK. committing unit tests that failure onto TRUNK means that gump will fail to build JCL. last time that happened, there were literally hundreds of dependent products that could not be built. each failed project means an email every day until it's fixed. thus, a gump storm. Wow. That's a shame. I'd think not being able to add unit tests that fail to the main line would tend to lead to a lot fewer unit tests. BTW, a couple weeks back I added a unit test patch to the JBoss Memory Leak bug. The added test will fail, so the patch shouldn't be committed to trunk. (Thus confirming my point above). Brian - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do you Yahoo!? Make Yahoo! your home page http://www.yahoo.com/r/hs - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] possible errors in child first cases [WAS Re: [logging] issues highlighted by analysis]
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Sun, 2005-04-24 at 23:59 -0700, Brian Stansberry wrote: --- robert burrell donkin snip When I ran these, for every child-first case the sysout output said the caller was defined by the child loader. [java] Callerdefined by CHILD class loader For 18, 20, 26, 28, 30 and 32, the overview says caller should be loaded by the parent. Haven't had a chance to try to see why, but this could be the cause of some problems. that's interesting. i've rerun the tests on my machine and all those tests seem to have the caller defined correctly. (i don't seem to have anything which isn't check in.) i'm running Java HotSpot(TM) Client VM (build 1.4.2_04-b05, mixed mode) on linux at the moment. anyone else experiencing these problems? Please note that the results I reported came when I ran the tests using the patched version of LogFactoryImpl I discussed. Don't know if that would matter or not. I can try again tonight using the normal JCL. Or it could be some other screwup on my part. Brian - robert [java] Running case 18... [java] Child context classloader: false [java] Child first: true [java] JCL defined by CHILD class loader [java] Log4j defined by CHILD class loader [java] Static Logger defined by CHILD class loader [java] Callerdefined by PARENT class loader [java] Running case 20... [java] Child context classloader: true [java] Child first: true [java] JCL defined by CHILD class loader [java] Log4j defined by CHILD class loader [java] Static Logger defined by CHILD class loader [java] Callerdefined by PARENT class loader [java] Running case 26... [java] Child context classloader: false [java] Child first: true [java] JCL defined by CHILD class loader [java] Log4j defined by CHILD class loader [java] Static Logger defined by CHILD class loader [java] Callerdefined by PARENT class loader [java] Running case 28... [java] Child context classloader: true [java] Child first: true [java] JCL defined by CHILD class loader [java] Log4j defined by CHILD class loader [java] Static Logger defined by CHILD class loader [java] Callerdefined by PARENT class loader [java] Running case 30... [java] Child context classloader: false [java] Child first: true [java] JCL defined by CHILD class loader [java] Log4j defined by CHILD class loader [java] Static Logger defined by CHILD class loader [java] Callerdefined by PARENT class loader [java] Running case 32... [java] Child context classloader: true [java] Child first: true [java] JCL defined by CHILD class loader [java] Log4j defined by CHILD class loader [java] Static Logger defined by CHILD class loader [java] Callerdefined by PARENT class loader - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do you Yahoo!? Yahoo! Mail - You care about security. So do we. http://promotions.yahoo.com/new_mail - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] issues highlighted by analysis
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Sat, 2005-04-23 at 14:12 +1200, Simon Kitching wrote: On Fri, 2005-04-22 at 08:54 -0700, Brian Stansberry wrote: --- Simon Kitching [EMAIL PROTECTED] wrote: On Tue, 2005-04-19 at 21:26 +0100, robert burrell donkin wrote: here are my results obtained by comparing the theory with the current behaviour. (1) JCL Behaves As Expected (50%) - 1,2,3,4,7,8,11,12,15,16,17,18,19,23,27,31 (2) JCL Throws Exception Rather Than Logging To Log4J (28%) --- 5,6,13,14,21,22,24,25,29 (3) JCL Throws Exception Rather Than Logging To java.util.logging (22%) --- 9,10,20,26,28,30,32 Where are these scenarios (1-32) documented? They're in the demonstration folder in svn. May be another way to get to it, but this works: http://svn.apache.org/viewcvs.cgi/jakarta/commons/proper/logging/trunk/demonstration/src/java/overview.html?rev=159142view=log Thanks, found them. i now have unit tests that replicate the failure cases. That's great. i'm wondering whether to commit them onto a branch so that everyone can take a look, check their accuracy and take a look at fixes. opinions? Please forgive if this is a stupid question, but why on a branch? Brian - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] issues highlighted by analysis
--- Simon Kitching [EMAIL PROTECTED] wrote: On Tue, 2005-04-19 at 21:26 +0100, robert burrell donkin wrote: here are my results obtained by comparing the theory with the current behaviour. (1) JCL Behaves As Expected (50%) - 1,2,3,4,7,8,11,12,15,16,17,18,19,23,27,31 (2) JCL Throws Exception Rather Than Logging To Log4J (28%) --- 5,6,13,14,21,22,24,25,29 (3) JCL Throws Exception Rather Than Logging To java.util.logging (22%) --- 9,10,20,26,28,30,32 Where are these scenarios (1-32) documented? They're in the demonstration folder in svn. May be another way to get to it, but this works: http://svn.apache.org/viewcvs.cgi/jakarta/commons/proper/logging/trunk/demonstration/src/java/overview.html?rev=159142view=log Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] update docs to specify that java 1.1 is supported
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Sat, 2005-04-16 at 00:18 -0700, Brian Stansberry wrote: --- Simon Kitching [EMAIL PROTECTED] wrote: On Fri, 2005-04-15 at 00:02 -0700, Brian Stansberry snip 3) Did a little archeology and it looks like JCL 1.0.1 was cut about a week before the AccessController stuff was added. So that's the last release that ran on JDK 1.1. Yep, that's how I read the CVS/SVN logs too. Version 1.0.2 was JDK1.2 only. There's no information I can find on whether the change to drop JDK 1.1 was deliberate or not.. For odd reasons (see below) I was reading Bugzilla 10825, and in the bug's discussion thread there are comments made just a few weeks before the AccessController stuff was added that JDK 1.1.8 compatibility was important. the loss of support was accidental: i would definitely have -1'd the release (which would have sunk it) had i know about the loss of java 1.1 support. (at the very least, the numbering rules mean that it should have to have been released as JCL 1.1 or JCL 2 rather than 1.0.2.) BTW, I forgot to mention this in my earlier comments on running w/ JDK 1.1. For both JCL 1.0.1 and 1.0 the binary download didn't run (got a NoClassDefFoundError looking for LogFactory). I had to rebuild from source to get them to run. I'm *speculating* the binaries were compiled targetting JDK 1.2 and the 1.1 JVM reacted by throwing a NoClassDefFoundError. I know later versions of the JDK give you a message saying they don't understand the class format; maybe 1.1 didn't do that?? I point this out because if we put a note in the wiki saying 1.0.1 and earlier work with JDK 1.1 we should probably be sure the download binaries work :) Brian - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] update docs to specify that java 1.1 is supported
--- robert burrell donkin [EMAIL PROTECTED] wrote: snip this week, i plan to start getting things moving forward again from the analysis document i published. i'm taking a look at the patches tonight (thanks for them :) i'll pull together some results and take a look at listing the bugs. maybe i'll put them in bugzilla so that folks can contributing patches more easily and so discussion is automatically tied together. i have hopes that many of these can be fixed (and fixed without too much trouble). there are also a couple of other tasks on my list. i now think that a build which creates just an impl jar is required to support at least one use case. anyone want to grab this task? Sure, I can do that. You want patches for the 1.0.5 branch as well as trunk, or just trunk? I'm concerned about the memory leak problem with the impl jar, but it looks like truly solving that will have to wait for the addition of something like Simon's ContextClassLoaderLocal to the JDK. (BTW, any news on that?) I also owe a patch to the user guide discussing best practices re not caching Log in a static field. If no one objects, I can also include comments on JDK 1.1 support. Should be able to do these in the next few evenings. Brian __ Do you Yahoo!? Read only the mail you want - Yahoo! Mail SpamGuard. http://promotions.yahoo.com/new_mail - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] update docs to specify that java 1.1 is supported
--- Simon Kitching [EMAIL PROTECTED] wrote: On Fri, 2005-04-15 at 00:02 -0700, Brian Stansberry wrote: Random thoughts 1) Found a brief discussion on this list a month ago that touched on issues with JDK detection: http://marc.theaimsgroup.com/?l=jakarta-commons-devm=111049144008136w=2 Thanks for the link. 2) My experience is that the JVM will not try to load a class until it is needed, so even though AccessController and PrivilegedAction are imported, LogFactory/LogFactoryImpl/SimpleLogcan be loaded. In the test I ran the NoClassDefFoundError is thrown when an attempt is made to use PrivilegedAction, not when LogFactory is loaded. So, *potentially* the error can be caught and handled or just avoided via JDK detection. Yep, that's my understanding of the Sun JVM at least.. The concern I have is that this lazy loading of classes is not required in the spec (see http://java.sun.com/docs/books/vmspec/2nd-edition/html/Concepts.doc.html#22574 ). Some JVMs (e.g. the M$ JVM used in Win9x browsers) *may* try to link all classes when LogFactory is loaded, in which case JDK detection won't work unless we use reflection for the AccessController stuff. Reflection sounds pretty nasty. (Is it even possible for anonymous inner classes? Don't know how it could be.) Yep, I agree with this too. In particular, assuming that lazy loading is in effect might get hairy with GCJ. I think we would have to go with a reflection-based approach somehow. I don't think the anonymous-inner-class bit is a show-stopper; can't we just pull that out into a separate, ordinary, class? Yes, of course (sound of palm slapping forehead) :) However we need to invoke it moderately frequently (each time LogFactory.getLog is called). So I'm not sure normal reflection will be acceptable. Particularly given your point a week or two ago about libraries not caching Logs in static fields. Caching in instance fields will increase the number of LogFactory.getLog calls. 3) Did a little archeology and it looks like JCL 1.0.1 was cut about a week before the AccessController stuff was added. So that's the last release that ran on JDK 1.1. Yep, that's how I read the CVS/SVN logs too. Version 1.0.2 was JDK1.2 only. There's no information I can find on whether the change to drop JDK 1.1 was deliberate or not.. For odd reasons (see below) I was reading Bugzilla 10825, and in the bug's discussion thread there are comments made just a few weeks before the AccessController stuff was added that JDK 1.1.8 compatibility was important. 4) Throwing in my own 2 cents, I think of JCL as being targetted to component/library/framework developers. I don't expect there are many applets being written that target 1.1 and also incorporate the kinds of libraries that use JCL. Well, as I said in my earlier email, Win9x (and possible WinNT) shipped with the microsoft 1.1 JVM, and I believe a fair number of simple applets still target that JVM in order to avoid requiring users of those operating systems to install a JVM explicitly. My current opinion, though, is that we should: (a) simply document the current state in the commons-logging website and/or wiki: JCL 1.0.2 and later require JVM1.2. +1 .except I just tried running JCL 1.01 using JDK 1.1 and it failed with an NPE. This was Bugzilla 10825 which was corrected in 10/2002. As I understand it, basically JCL will fail under JDK 1.1 if it is loaded by the system classloader (which it is in my simple test). Just tried JCL 1.0 and had the same problem. I suppose in the applet-running-in-Windows scenario that we are most concerned about JCL wouldn't be loaded by the system classloader, so the bug may not apply. (b) when working on the revised JCL, regard support for 1.1 as desirable but not mandatory. Sounds good to me. Perhaps some of Robert's ideas about lifting off a LogFactory superclass will open a path to providing support to 1.1 without having to overly encumber the normal discovery process. I think at this point the work we already have in progress for JCL is enough without tackling this task too. +1 I know that for me personally spending the last couple evenings digging into the intricacies of getting things to work on JDK 1.1 has pretty well exhausted my appetite for the topic ;) Brian __ Do you Yahoo!? Yahoo! Small Business - Try our new resources site! http://smallbusiness.yahoo.com/resources/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] update docs to specify that java 1.1 is supported
--- Simon Kitching [EMAIL PROTECTED] wrote: Hmm..you're quite right. The problem is not getContextClassLoader, but there *is* a problem with AccessController. I've checked the svn/cvs history, and the AccessController stuff was added by Richard Sitze in r138923 on 2002-10-18. Presumably that was the date at which JCL no longer worked on java1.1. Given that there have been no complaints about that, should we assume that JCL for java-1.1 is not in great demand, and explicitly support only 1.3 or later? I see no reason to support 1.2, as it is well out-of-date; 1.1 has *some* justification as many Win9x systems shipped with this jvm version and still are in use (see the user request which started this discussion). ALternatively, we can simply test for 1.1 before attempting to use the AccessController stuff... Random thoughts 1) Found a brief discussion on this list a month ago that touched on issues with JDK detection: http://marc.theaimsgroup.com/?l=jakarta-commons-devm=111049144008136w=2 2) My experience is that the JVM will not try to load a class until it is needed, so even though AccessController and PrivilegedAction are imported, LogFactory/LogFactoryImpl/SimpleLogcan be loaded. In the test I ran the NoClassDefFoundError is thrown when an attempt is made to use PrivilegedAction, not when LogFactory is loaded. So, *potentially* the error can be caught and handled or just avoided via JDK detection. The concern I have is that this lazy loading of classes is not required in the spec (see http://java.sun.com/docs/books/vmspec/2nd-edition/html/Concepts.doc.html#22574 ). Some JVMs (e.g. the M$ JVM used in Win9x browsers) *may* try to link all classes when LogFactory is loaded, in which case JDK detection won't work unless we use reflection for the AccessController stuff. Reflection sounds pretty nasty. (Is it even possible for anonymous inner classes? Don't know how it could be.) 3) Did a little archeology and it looks like JCL 1.0.1 was cut about a week before the AccessController stuff was added. So that's the last release that ran on JDK 1.1. 4) Throwing in my own 2 cents, I think of JCL as being targetted to component/library/framework developers. I don't expect there are many applets being written that target 1.1 and also incorporate the kinds of libraries that use JCL. Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] update docs to specify that java 1.1 is supported
LogFactory relies on Thread.getContextClassLoader(), which didn't exist in the 1.1 JVM. So, I wouldn't expect JCL to run. I played around with testing this a while back (downloaded Sun's 1.1 JVM), but hit some minor roadblock and stopped. You're right -- this should be clarified, particularly since it also impacts design issues. Tonight I'll get the test working. Brian --- Simon Kitching [EMAIL PROTECTED] wrote: Hi, A user recently asked on the commons-user list whether JCL runs on java 1.1. I'm sure it is meant to, but I can't find anywhere in the docs myself that say what JVMs are supported. So attached is a proposed patch to clarify this in the docs. Is everyone happy with this? Cheers, Simon Index: xdocs/index.xml === --- xdocs/index.xml (revision 161185) +++ xdocs/index.xml (working copy) @@ -39,6 +39,9 @@ and contributors may write Log implementations for the library of their choice./p +pJakarta Commons Logging supports all versions of java equal to or later +than java 1.1./p + /section Index: xdocs/guide.xml === --- xdocs/guide.xml (revision 161185) +++ xdocs/guide.xml (working copy) @@ -92,6 +92,10 @@ logging abstraction, that allows the user (application developer) to plug in a specific logging implementation. /p + +pJCL supports all versions of java equal to or later +than java 1.1./p + pJCL provides thin-wrapper codeLog/code implementations for other logging tools, including a href=http://jakarta.apache.org/log4j/docs/index.html;Log4J/a, Index: src/java/org/apache/commons/logging/package.html === --- src/java/org/apache/commons/logging/package.html (revision 161185) +++ src/java/org/apache/commons/logging/package.html (working copy) @@ -43,6 +43,8 @@ System.err./li /ul +pThis library is intended to run on any JVM equal to or later than +version 1.1./p h3Quick Start Guide/h3 - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] update docs to specify that java 1.1 is supported
No joy. Doesn't run under JDK 1.1. I wrote a simple
main method that calls LogFactory.getLog() and then
Log.info(). Call to LogFactory.getLog() fails with a
NoClassDefFoundError: java/security/PrivilegedAction.
java.security.AccessController isn't in 1.1 either.
Brian
--- Simon Kitching [EMAIL PROTECTED] wrote:
Hi Brian,
This code in LogFactory:
public static LogFactory getFactory()
throwsLogConfigurationException {
// Identify the class loader we will be
using
ClassLoader contextClassLoader =
(ClassLoader)AccessController.doPrivileged(
new PrivilegedAction() {
public Object run() {
return
getContextClassLoader();
}
});
actually calls a method named
getContextClassLoader defined in the
LogFactory class, *not*
Thread.getContextClassLoader. The local
getContextClassLoader method uses reflection to
handle 1.1 jvms. On
1.1 JVMs, the classloader which loaded the current
class is always
returned (see catch(NoSuchMethodException e) on
line 551 of
LogFactory.java).
So I *think* everything currently works ok on 1.1
jvms. I haven't tested
it myself, though, so would be very interested in
results of your
testing.
Can you even *download* 1.1 JVMs these days??
Cheers,
Simon
PS: I'm back from my holidays now, and ready to get
stuck into JCL
(well, once recovered from my jetlag!).
On Wed, 2005-04-13 at 08:50 -0700, Brian Stansberry
wrote:
LogFactory relies on
Thread.getContextClassLoader(),
which didn't exist in the 1.1 JVM. So, I wouldn't
expect JCL to run. I played around with testing
this
a while back (downloaded Sun's 1.1 JVM), but hit
some
minor roadblock and stopped. You're right -- this
should be clarified, particularly since it also
impacts design issues. Tonight I'll get the test
working.
Brian
--- Simon Kitching [EMAIL PROTECTED] wrote:
Hi,
A user recently asked on the commons-user list
whether JCL runs on java
1.1. I'm sure it is meant to, but I can't find
anywhere in the docs
myself that say what JVMs are supported.
So attached is a proposed patch to clarify this
in
the docs.
Is everyone happy with this?
Cheers,
Simon
Index: xdocs/index.xml
===
--- xdocs/index.xml (revision 161185)
+++ xdocs/index.xml (working copy)
@@ -39,6 +39,9 @@
and contributors may write Log implementations
for
the library of
their choice./p
+pJakarta Commons Logging supports all
versions of
java equal to or later
+than java 1.1./p
+
/section
Index: xdocs/guide.xml
===
--- xdocs/guide.xml (revision 161185)
+++ xdocs/guide.xml (working copy)
@@ -92,6 +92,10 @@
logging abstraction, that allows the user
(application developer) to plug in
a specific logging implementation.
/p
+
+pJCL supports all versions of java equal to
or
later
+than java 1.1./p
+
pJCL provides thin-wrapper codeLog/code
implementations for
other logging tools, including
a
href=http://jakarta.apache.org/log4j/docs/index.html;Log4J/a,
Index:
src/java/org/apache/commons/logging/package.html
===
---
src/java/org/apache/commons/logging/package.html
(revision 161185)
+++
src/java/org/apache/commons/logging/package.html
(working copy)
@@ -43,6 +43,8 @@
System.err./li
/ul
+pThis library is intended to run on any JVM
equal
to or later than
+version 1.1./p
h3Quick Start Guide/h3
-
To unsubscribe, e-mail:
[EMAIL PROTECTED]
For additional commands, e-mail:
[EMAIL PROTECTED]
__
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam
protection around
http://mail.yahoo.com
__
Do you Yahoo!?
Yahoo! Small Business - Try our new resources site!
http://smallbusiness.yahoo.com/resources/
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
[logging] Static references to Log objects
From the thread Re: Idea: combine JCL 2.0 and UGLI in
Logging Services' CL2...
--- Simon Kitching [EMAIL PROTECTED] wrote:
snip
Remy Maucherat wrote:
(suggesting it is a good idea, on any logging
framework, to call
getLogger inside your app's critical path is quite
funny).
Really? When getLogger can return different Log
objects depending upon
the current context classloader? No class that could
be deployed via a
shared classloader should cache a Log object in a
static field...
Very true. And since the intended users of JCL are
reusable components that could be deployed in a shared
classloader, your statement implies that in the uses
for which JCL is intended, no static references to Log
objects should be kept.
I think this point certainly deserves mention in the
JCL user guide (I'll submit a patch).
This line of thought led me to reconsider an idea I'd
rejected a couple weeks back related to the JCL memory
leak problem. Basically the leak occurs if
LogFactoryImpl is defined by a parent classloader
while the class of one of its Log instances is defined
by a child loader. The chain of references from the
Log instance to its classloader will prevent gc of the
child loader on undeploy.
This chain could (potentially) be broken if
LogFactoryImpl only held WeakReferences to its Log
instances as follows:
public Log getInstance(String name) throws
LogConfigurationException {
-
-Log instance = (Log) instances.get(name);
-if (instance == null) {
- instance = newInstance(name);
- instances.put(name, instance);
+
+Log instance = null;
+WeakReference ref = (WeakReference)
instances.get(name);
+if (ref != null) {
+ instance = (Log) instances.get(name);
+ if (instance == null) {
+instance = newInstance(name);
+instances.put(name, new
WeakReference(instance));
+ }
}
return (instance);
I'd rejected this approach because:
1) It adds overhead to getInstance().
2) It adds a dependency on JDK 1.2 (although JCL has
others).
3) If calling code does not cache the Log object, it
may be recreated multiple times as the WeakReference
is cleared. Don't know if this is a serious issue in
the real world.
4) Most importantly, this approach is based on the
idea that all objects holding a hard reference to a
Log will be gc'ed on undeploy, allowing the
WeakReference to clear. This will fail if even 1
static reference to a Log whose class was defined by
the child loader is held somewhere.
So, negative performance implications + prone to not
working = bad idea. But, Simon's comment on caching
Log objects in static fields led me to reconsider
enough to throw the concept out to the community for
comment.
Or maybe it's just that the memory leak issue is what
led me to wander into JCL-land in the first place and
now it's become my great white whale...
Brian
__
Do you Yahoo!?
Make Yahoo! your home page
http://www.yahoo.com/r/hs
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] Static references to Log objects
--- Brian Stansberry [EMAIL PROTECTED]
wrote:
snip
This line of thought led me to reconsider an idea
I'd
rejected a couple weeks back related to the JCL
memory
leak problem. Basically the leak occurs if
LogFactoryImpl is defined by a parent classloader
while the class of one of its Log instances is
defined
by a child loader. The chain of references from the
Log instance to its classloader will prevent gc of
the
child loader on undeploy.
This chain could (potentially) be broken if
LogFactoryImpl only held WeakReferences to its Log
instances as follows:
public Log getInstance(String name) throws
LogConfigurationException {
-
-Log instance = (Log) instances.get(name);
-if (instance == null) {
- instance = newInstance(name);
- instances.put(name, instance);
+
+Log instance = null;
+WeakReference ref = (WeakReference)
instances.get(name);
+if (ref != null) {
+ instance = (Log) instances.get(name);
+ if (instance == null) {
+instance = newInstance(name);
+instances.put(name, new
WeakReference(instance));
+ }
}
return (instance);
Oops. Try:
public Log getInstance(String name) throws
LogConfigurationException {
-
-Log instance = (Log) instances.get(name);
-if (instance == null) {
- instance = newInstance(name);
- instances.put(name, instance);
+
+Log instance = null;
+WeakReference ref = (WeakReference)
instances.get(name);
+if (ref != null) {
+ instance = (Log) instances.get(name);
+}
+if (instance == null) {
+ instance = newInstance(name);
+ instances.put(name, new
WeakReference(instance));
+}
return (instance);
Brian
__
Yahoo! Messenger
Show us what our next emoticon should look like. Join the fun.
http://www.advision.webevents.yahoo.com/emoticontest
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: Idea: combine JCL 2.0 and UGLI in Logging Services' CL2
--- Simon Kitching [EMAIL PROTECTED] wrote: snip Surely tomcat could have a container/lib directory in which jar files are visible to tomcat but not to the webapps? Putting jcl in here instead of shared/lib would then make logging in individual webapps much more predictable. Yes, webapps would then have to provide commons-logging.jar themselves in WEB-INF/lib, and that would cause duplicate copies in memory, but that also means that the individual webapps are properly isolated from each other. Optimisations with potentially complex side-effects (libs in a shared classloader) should be an *optional* step, not the default for the container. A tricky aspect of Tomcat is that besides providing container services, it also provides code that runs inside the individual web applications (e.g. JSP servlet and the DefaultServlet for serving static content). So, while in general Tomcat acts more like an application and could probably pick whatever logging framework it wants, some portions of its code are more like a library and should use a logging abstraction like JCL. These portions of the code need to be in the shared classloader, and the commons-logging-api.jar needs to be available to them. Brian __ Do you Yahoo!? Yahoo! Personals - Better first dates. More second dates. http://personals.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] distribution packaging
--- Ceki Gülcü [EMAIL PROTECTED] wrote: snip Both you and Brian refer to my analysis of JCL as Ceki's document without actually linking to it. Is it because my document is so notorious that it does not need linking to? :-) My apologies. I originally wrote mine in Word and neglected to convert the reference to your document to a proper link when I converted to HTML. Fixed now. Guess it's time to get a proper web page dev tool :) Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] demonstration code
--- robert burrell donkin [EMAIL PROTECTED] wrote: On Sun, 2005-03-20 at 22:22 -0800, Brian Stansberry wrote: --- robert burrell donkin [EMAIL PROTECTED] wrote: inspired by ceki's examples, i've been working on an extended systematic analysis of the various classloader cases. there's demonstration code for them as well as documentation. Look forward to seeing this. I was thinking of getting going on some unit tests based on the earlier analyses, but think I'll wait a bit to see what you've done. i would recommend waiting. No problem. it should be quite easy to add extra analysis cases in the same style to the code base when it's done. i haven't covered much of the ground you had started to explore (nor the ground suggested by simon concerning unconventional classloaders). it would great if these could be added. it may be a day or three before it's ready. i don't particularly want to rush. it's easy to make mistakes when analysing these issues and it's easy to then build misleading arguments upon shaky foundations. +1 i hope that people will bring any mistakes i make to our attention as quickly as possible. i haven't heard any feedback about: http://jakarta.apache.org/commons/logging/tech.html. i'll be relying on the concepts and terminology outlined there in the analysis and demonstration code. again, i'd like to encourage anyone who finds any inaccuracies to speak up as soon as possible. Looks good and is much appreciated. I expect when I get into classloading discussions at work I'll refer people to that page as an excellent reference document. In discussions on this list I'm sure the precision of using terms like defining classloader vs initializing classloader will at times come in handy. This is a bit of a nit, but one thing I noticed was the emphasis on creator and created in the discussion of the thread context class loader. Setting of the TCCL is not necessarily closely associated with thread creation but can also be associated with thread execution crossing a significant application boundary. For example in Tomcat a pool of threads is created (I believe by the connector code). Not sure if any TCCL is set at that point, but if one is it's for sure not any web app's classloader. Once a thread has been assigned to handle a request, execution moves through the various TC container layers (Engine, Host) and only when execution is about to enter the Context layer is the TCCL set to the web app's classloader. When the call to the Context layer returns the TCCL is set back to what it was before. A similar approach is followed in JBoss. Interestingly, the Javadoc for Thread.setContextClassLoader() itself emphasizes the relationship between thread creation and setting the TCCL, but this is somewhat misleading. Brian __ Do you Yahoo!? Yahoo! Small Business - Try our new resources site! http://smallbusiness.yahoo.com/resources/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging, beanutils, others] Initial proposal for java.lang.ContextClassLoaderLocal or java.lang.Singleton
--- Phil Steitz [EMAIL PROTECTED] wrote: Thanks, Simon. Much clearer now. snip/ No, I mean the situation where a container continues running, while a webapp or EJB running within it is stopped restarted (potentially with updated code). OK, so then lifecycle events should get triggered. In the case of EJBs, are there any deployment-scope lifecycle callbacks analogous to the Servlet API's ServletContextListener? All the callbacks I'm aware of are scoped to individual beans within the pool, not to the pool itself or to the overall EJB jar deployment. If, for example, a stateless session bean used the commons-foobar library which in turn used JCL for logging, we wouldn't want the JCL cleanup code to be called each time ejbRemove() was invoked. Particularly not if the SLSB was just one of many different beans deployed together, all of which somehow use JCL. Brian __ Do you Yahoo!? Yahoo! Mail - now with 250MB free storage. Learn more. http://info.mail.yahoo.com/mail_250 - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] demonstration code
--- robert burrell donkin [EMAIL PROTECTED] wrote: snip the link between the concept of application boundaries, threads and containers is missing and should really be spelt out. from a narrative perspective, it's probably important to give the specification and then explain that creator (in the sense of the thread context classloader documentation) in the case of a container (or any other thread pooled application) encompasses the concept of owner. Ah, owner. That's the word I wanted. I was thinking about wording when I posted last night and kept thinking about the thread's manager but didn't like that. so, the owner of the thread should ensure that a thread has an appropriate classloader set for the application currently executing in that thread. this can then link into the (missing) subject of application boundaries. in addition, it's probably worth including something of craig's explanation about the evolution of class-first classloading in the J2EE section. JCL needs to run on older containers and so people need to be aware of this. i takes me a lot of energy to write documentation and the context hierarchical section didn't get as much as the preceding section. it could probably do with revisiting but i'm not sure when i'll find the time. so, i'd be grateful for patches. I'd be happy to try to come up with something. I know what you mean about the energy to write docs -- I wanted to suggest some wording with my last message, but had no creative energy at the time. i think that the consensus is that ensuring that applications release logging (and other resources) correctly is something that applications should definitely be taking seriously. it's worth explaining this in the user guide. again, i'd be grateful for patches. +1 Every now and then I think about puttin up a web page devoted to known memory leaks in Java libraries and how to prevent them. Kind of doubt I'll find the time, though. Brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] demonstration code
--- robert burrell donkin [EMAIL PROTECTED] wrote: inspired by ceki's examples, i've been working on an extended systematic analysis of the various classloader cases. there's demonstration code for them as well as documentation. it's not complete but most of the substantial cases address by ceki's document are covered. (the unconventional context classloader cases highlighted by simon are missing but they need a little more consideration.) i think that developers should find it interesting and maybe a little educational. it certainly help me think a lot more clearly about these issues. Look forward to seeing this. I was thinking of getting going on some unit tests based on the earlier analyses, but think I'll wait a bit to see what you've done. my plan is to establish a baseline without fixing bugs so that theoretical optimum behaviour can be compared to where JCL is now. it includes configurations which cannot work in theory as well as those which can. i'm now trying to work out the best place and packaging for the source. in some ways, it's not really JCL source and so in some ways, it doesn't seem to belong in the JCL. on the other hand, i hope that it might be useful for JCL advanced users and developers.so, there a temptation to include it (perhaps within a subdirectory). opinions? I definitely think this kind of thing should go into a shared repository somewhere and JCL seems like the logical place. Publishing zips is pretty cumbersome. Brian - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do you Yahoo!? Yahoo! Small Business - Try our new resources site! http://smallbusiness.yahoo.com/resources/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [beanutils] [logging]j2ee unit tests added: memory leak demonstrated (?)
Replying to myself: --- Brian Stansberry [EMAIL PROTECTED] wrote: Simon, I spent some time last night looking at this, mostly just familiarizing myself with how beanutils works so I could understand the problem. I think you're right to be concerned about JCL, as from what I can see the two applications are very similar: weak-keyed map (aka WeakMap) held statically by class loaded by container (beanutils: BeanUtilsBean.beanByClassLoader; JCL: LogFactory.factories). WeakMap key is the TCCL. WeakMap value is a class loaded by container, but value is instantiated while component loader is the TCCL (beanutils: BeanUtilsBean; JCL: LogFactoryImpl). WeakMap value holds another map (aka StrongMap) (beanutils: BeanUtilsBean.convertUtilsBean.converters; JCL: LogFactory.instances) StrongMap value is a class loaded by the component loader (beanutils: FloatConverter; JCL: Log4jLogger). Class implements an interface loaded by the container loader (beanutils: Converter; JCL: Log). This reference is likely what's preventing release of the classloader in beanutils. After exploring more with the JCL code, I'm almost certain the reference to FloatConverter in BeanUtilsBean.convertUtilsBean.converters is what's causing o.a.c.beanutils.converter.MemoryTestCase to fail. When I wrote before, I only said likely because I couldn't see why JCL wouldn't always have the same failure, and I had tests where it didn't. But I've now created tests where it does (see below); tests that are basically analogous to MemoryTestCase. I noticed that the JCL unit test I wrote for memory release has a weakness in that it doesn't add the Log instance to LogFactoryImpl.instances. I noticed that a week ago and added the required call so I could check the tests still passed. They did. But, didn't get a chance to clean up the code and submit a proper patch for the tests. My bad :( I'll for sure do that tonight, plus add some more assertions like the beanutils test has in order to ensure that classes are being loaded by the intended loader. This should either surface a problem in JCL or help shed some light on the beanutils problem. Will have to be another day before I submit a patch for the JCL tests (after midnight now). But, I've found in JCL the classloader is not released when LogFactoryImpl is loaded by a parent loader and the Log wrapper is loaded by a child. I've identified two basic configurations where this might happen: 1) Parent-first delegation model, where the parent has commons-logging-api.jar, child has commons-logging.jar and child wants to use Log4j or Avalon (or some custom Log implementation). 2) Child-first delegation model that uses the configuration I proposed in my Further Analysis document; i.e. where the parent has commons-logging-api-xxx.jar, child has commons-logging-impl.jar and child wants to use Log4j or Avalon. Brian __ Do you Yahoo!? Make Yahoo! your home page http://www.yahoo.com/r/hs - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [beanutils] [logging]j2ee unit tests added: memory leak demonstrated (?)
--- Simon Kitching [EMAIL PROTECTED] wrote: snip I should have been a little clearer about MemoryTestCase. There are two tests that are failing: * testComponentRegistersCustomConverter This fails for a reasonably obvious reason, and it's the same one that is already documented in the javadoc for JCL's WeakHashtable class. Unfortunately as this test shows, it will be encountered quite often when people use beanutils converter functionality in a j2ee-like environment. * testComponentRegistersStandardConverter This fails for no reason I can understand. It is very similar to testComponentRegistersCustomConverter except that it does not involve loading a class via a child classloader, and therefore should not trigger the bug I believe exists. And yes, commending out the reference to FloatConverter in the testComponentRegistersStandardConverter test does make it pass; the million-dollar question is: why does the test fail when FloatConverter is used? Maybe it's because when running maven test, the unit test code is actually run within a custom classloader? Does maven itself use BeanUtils? Hmm.. Ahh, I had run the tests inside Eclipse, and testComponentRegistersStandardConverter passed. Could very well be due to maven (which I know very little about). Just to add further confusion, I added a target to the beanutils build.xml to run the MemoryTestCase, and testComponentRegistersStandardConverter failed. I threw in a System.out.println to record the classname of origContextClassLoader; in Eclipse it's sun.misc.Launcher$AppClassLoader; in both maven and ant it's org.apache.tools.ant.loader.AntClassLoader. Brian __ Do you Yahoo!? Make Yahoo! your home page http://www.yahoo.com/r/hs - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [beanutils] j2ee unit tests added: memory leak demonstrated (?)
Simon, I spent some time last night looking at this, mostly just familiarizing myself with how beanutils works so I could understand the problem. I think you're right to be concerned about JCL, as from what I can see the two applications are very similar: weak-keyed map (aka WeakMap) held statically by class loaded by container (beanutils: BeanUtilsBean.beanByClassLoader; JCL: LogFactory.factories). WeakMap key is the TCCL. WeakMap value is a class loaded by container, but value is instantiated while component loader is the TCCL (beanutils: BeanUtilsBean; JCL: LogFactoryImpl). WeakMap value holds another map (aka StrongMap) (beanutils: BeanUtilsBean.convertUtilsBean.converters; JCL: LogFactory.instances) StrongMap value is a class loaded by the component loader (beanutils: FloatConverter; JCL: Log4jLogger). Class implements an interface loaded by the container loader (beanutils: Converter; JCL: Log). This reference is likely what's preventing release of the classloader in beanutils. I noticed that the JCL unit test I wrote for memory release has a weakness in that it doesn't add the Log instance to LogFactoryImpl.instances. I noticed that a week ago and added the required call so I could check the tests still passed. They did. But, didn't get a chance to clean up the code and submit a proper patch for the tests. My bad :( I'll for sure do that tonight, plus add some more assertions like the beanutils test has in order to ensure that classes are being loaded by the intended loader. This should either surface a problem in JCL or help shed some light on the beanutils problem. Regards, Brian --- Simon Kitching [EMAIL PROTECTED] wrote: Hi, I've just added a unit test o.a.c.beanutils.converter.MemoryTestCase. This test case currently fails, demonstrating (I believe) that there is a serious memory leak in beanutils in j2ee-like environments when components are undeployed. The tests show that if a Converter is registered while Thread.getContextClassLoader is set to a component-specific classloader, then when the component is undeployed that classloader cannot be garbage-collected. The problem is: I can't spot what is *causing* this bug. It would be great if someone else could have a look too. Given the new code in commons-logging that is intended to address a similar issue, I think this is worth resolving before releasing commons-logging 1.0.5 Regards, Simon - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do you Yahoo!? Yahoo! Small Business - Try our new resources site! http://smallbusiness.yahoo.com/resources/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] 1.0.5: WeakHashtable
--- robert burrell donkin
[EMAIL PROTECTED] wrote:
On Tue, 2005-03-08 at 06:41, Simon Kitching wrote:
Should the WeakHashtable class be rolled into
commons-logging.jar?
It seems easier for users than remembering to
deploy the extra jar, and
should be feasable by having something like this
in
Hashtable foo;
String version =
System.getProperty(java.vm.specification.version);
if (versionLessThan(version, 1.3)) {
foo = new Hashtable();
} else {
// use reflection to create instance
foo = createWeakHashtable();
}
Or is the reason for having it separate because
there is a performance
hit when using it? If that is so, then file
guide.xml should document
that rather than saying always deploy it when
using java 1.3 or later.
there may be some performance hit but this should
only really happen the
first time that a Log is obtained for a new
classloader. i doubt that
there will be significant real world performance
degradation by using
this jar. be nice to have some figures, though.
there were two main reasons why it was issued as an
optional jar:
1. JCL has always tried hard to be compatible with
early JVMs
2. backwards compatibility is very important
I was looking at the javadoc for WeakReference and
Hashtable, and they look like they've actually both
had stable APIs since JDK 1.2. So, I downloaded a 1.2
SDK and confirmed that I was able to compile
WeakHashtable using it.
This doesn't contradict the point about preserving
compatibility with early JVM, as WeakHashtable won't
run under 1.1. But, the JCL docs and I believe the
user guide talk about needing 1.3 and it looks like
1.2 is sufficient.
the memory problem is only likely to manifest when
frequently hot
deploying in certain containers.
In regards to my comment above, I'm sure the large
group of container providers who are supporting JDK
1.2 will be thrilled to know they can use
WeakHashtable to fix their memory leaks ;-)
i'm a little
reluctant to use system
property version numbers (since they haven't always
been very reliable)
and prefer to give users the explicit choice as to
whether they risk
using the new code or not.
however, maybe it would be a reasonable tradeoff in
this case and i
would be willing to be convinced. (i tend to be very
conservation when
maintaining components with large installation
bases).
opinions anyone?
The way I've seen JDK detection done is by trying to
load a class that first appeared in the target JDK.
ClassLoader loader = getClass().getClassLoader();
boolean jdk12 = false;
try {
loader.loadClass(java.util.Collection);
jdk12 = true;
}
catch (Exception e) {
// ignore
}
BTW, I believe JDK detection is actually mildly safer
than what LogFactory is doing now, which in the
default configuration amounts to calling
Class.forName(o.a.c.l.i.WeakHashtable) and catching
any Exception. If, contrary to instructions,
commons-logging-optional.jar were on the classpath in
a JDK 1.1 environment, I believe this call would throw
a NoClassDefFoundError which would not be caught.
Brian
__
Do you Yahoo!?
Yahoo! Small Business - Try our new resources site!
http://smallbusiness.yahoo.com/resources/
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] parent-first classloaders
--- Simon Kitching [EMAIL PROTECTED] wrote: Hi Ceki, You mentioned in your page on JCL (http://www.qos.ch/logging/classloader.jsp): quote Jake also keeps reminding us on the log4j-dev mailing list that the child-parent delegation model is not the only model out there and that parent-first delegation model is alive and well. /quote Are you able to provide the names of some frameworks that use parent-first classloading? I'm curious to find out whether these are large + well-used frameworks or whether these are experimental and/or obsolete systems. I find it odd that a container would make it impossible for a contained application to override libraries that the parent provides. This seems rather fragile and inflexible to me; maybe reading the documentation for some projects which Jake has referred to will help me understand the reasons for using parent-first delegation. JBoss also defaults to parent-first delegation (their Tomcat integration overrides the standard TC classloading). To get child-first classloading you have to add class-loading java2ClassLoadingCompliance=false/ to your war's jboss-web.xml file. I believe the EJB spec mandates standard Java2 (i.e. parent-first) classloading as well. Best regards, Brian Regards, Simon - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do you Yahoo!? Yahoo! Small Business - Try our new resources site! http://smallbusiness.yahoo.com/resources/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] distribution packaging
--- Ceki Gülcü [EMAIL PROTECTED] wrote: Brian, From a cursory look at your document, I have to *speculate* that the changes you describe do not solve the core flaws in JCL but merely hide them by falling back on java.util.logging. However, I am only *speculating* as I have not had a chance to study your document with the care that it deserves. This is definitely the case with JCL 1.0.5RC1; the exceptions you noted were (largely) no longer thrown, but JCL fell back on java.util.logging. With the packaging changes, the logging was done using the expected implementation. I didn't just want to rely on my noticing the formatting differences in the log output between the different loggers, so the test cases also log the classname of the Log wrapper. After careful study of JCL, I am convinced that JCL is broken beyond hope. While its interfaces can be salvaged, its implementation must be thrown away entirely. While this opinion is not popular around here, it is based on verifiable facts, not wishful thinking that does not survive critical scrutiny. Perhaps coming down to a solution involving distributing multiple different jars, teaching users how to correctly deploy them, and then still having some use cases where JCL's discovery mechanism doesn't work qualifies as broken beyond hope. I'm actually still somewhat on the fence on this one. I think there are two issues here: 1) Does changing packaging actually solve some of the identified problems? This issue can and should be resolved empirically. 2) Is a proposed change so ugly/difficult to understand/limited in effectiveness that it's better to not bother and instead focus energy on a more radical solution? This is really a value judgement that IMHO can best be resolved through discussion within the community. My goal to this point has been to help clarify the empirical issues so that any discussion of the value judgements could proceed from a shared base of understanding. Regarding the issues of politics you raise, I don't really have the historical background to comment other than to say that referring to your Think Again article as a rant was uncalled for (and actually detracts from the content of the wiki page). (OK, someone out there, flame away :-) ). snip In late 1999, National Magazine published an article about a newly discovered Archaeoraptor fossil, calling it a true missing link demonstrating the relation between birds and dinosaurs, supposedly bringing to conclusion a debate raging since the 1860s. When XU XING, a Chineese palaeontologist, declared that the missing-link fossil acquired by National Geographic was a fake, the illustrious magazine rechecked their facts and admitted their mistake. They had invested considerably in the article and had already checked their facts. However, when XU XING's message arrived, they did not summarily dismiss it or ridicule his findings. They rechecked their facts. For the details of this fascinating story, please refer to [2]. Thanks for this link. I'd never heard this story. I'm also a bit of a Sinologist, so the background on the fossil trade in Liaoning is personally interesting. Brian Recently the ASF celebrated its 10th anniversary. IMHO, if the foundation is ever to celebrate its 100th anniversary, we better develop a better tradition for dealing with critical input. [1] http://wiki.apache.org/jakarta-commons/Commons_20Logging_20FUD [2] http://www.bbc.co.uk/science/horizon/2001/dinofooltrans.shtml On 2005-03-08 7:35:11, Brian Stansberry wrote: I was a little surprised myself, which is why I wanted to follow Ceki's good example and publish test cases that could easily be verified (or debunked) by others. -- Ceki Gülcü The complete log4j manual: http://www.qos.ch/log4j/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Celebrate Yahoo!'s 10th Birthday! Yahoo! Netrospective: 100 Moments of the Web http://birthday.yahoo.com/netrospective/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] distribution packaging
--- robert burrell donkin [EMAIL PROTECTED] wrote: (new thread on packaging) On Thu, 2005-03-03 at 05:34, Brian Stansberry wrote: What I've found is documented at http://xnet.wanconcepts.com/jcl/furtherAnalysis.html. we need to take a longer look into repackaging. i didn't expect that altering the distributions would work so well. I was a little surprised myself, which is why I wanted to follow Ceki's good example and publish test cases that could easily be verified (or debunked) by others. i'd definitely be willing to review patches if someone wants to volunteer to alter the build, create some good test cases and write up some documentation. I'd be happy to take that on if a consensus is reached that repackaging is the way to go. Might take me a bit of time though, as I'm fairly swamped. i'd always suspected that static binding was the only solution for several difficult cases (but using byte code engineering rather than selective compilation as used in UGLI). the sticking point for this (and many other cases i'd like to be able to address) is that LogFactory is too complex and too tightly coupled to a container environment. Yep, all the tests cases basically simulate different types of containers. But hopefully we're close to getting the container use cases clean. Brian - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Celebrate Yahoo!'s 10th Birthday! Yahoo! Netrospective: 100 Moments of the Web http://birthday.yahoo.com/netrospective/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] discovery error handling
--- robert burrell donkin [EMAIL PROTECTED] wrote: that all depends on what is meant by fix :) and on what is meant by soon :) anyone (brian or dennis, maybe?) want to volunteer to take a look into improving classloading for the 1.0.x development stream? I got some time to look into this and thought I'd start by going through the test cases Ceki Gülcü published (thanks Ceki). What I found was that 1) JCL 1.05RC1 goes a long way toward resolving the issues Ceki raised, and 2) providing a commons-logging-impl.jar (i.e. the commons-logging.jar classes not included in commons-logging-api.jar) for use in child deployments would resolve most of the rest. What I've found is documented at http://xnet.wanconcepts.com/jcl/furtherAnalysis.html. I know there has been a lot of discussion on this list of these issues, far more than I have had a chance to digest fully, so I apologize if I'm stating the obvious or missing the obvious. But, in any case I thought others might find it useful to have the relevant issues summarized in one document; I know I found Ceki's document very helpful. If anyone has any comments or suggestions, they would be most welcome. Best regards, Brian __ Celebrate Yahoo!'s 10th Birthday! Yahoo! Netrospective: 100 Moments of the Web http://birthday.yahoo.com/netrospective/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] 1.0.5 release plan
I'll have some time on Sunday and would be happy to help out with documentation if you'd like. Sorry, didn't get as much free time as I thought today; will get you some stuff tomorrow evening. I was building JCL with ant and couldn't get a commons-logging-optional.jar to build when executing the build.xml in the root directory. Instead the optional classes would get included in commons-logging.jar. Looked into this and saw that the when the main build.xml called the build in the optional folder, it was passing along all its properties, thus overriding the optional version. Attached is a patch that fixes this. I was also thinking a note in the user guide similar to the class javadoc in WeakHashtable might be useful. Or anything else you think appropriate. something for the user guide similar to the class javadocs together with some notes about the distribution (dropping the optional jar into the classpath) would be great :) Will do :) Brian __ Do you Yahoo!? All your favorites on one personal page Try My Yahoo! http://my.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging] 1.0.5 release plan
Hi Robert, Sorry to drop off the face of the earth -- my wife and I had a baby last month :) so . I'll have some time on Sunday and would be happy to help out with documentation if you'd like. Assuming you plan to format the Release Notes similarly to the 1.0.4 release, I can draft a couple paragraphs re: the change to LogFactory and the addition of WeakHashtable. I was also thinking a note in the user guide similar to the class javadoc in WeakHashtable might be useful. Or anything else you think appropriate. Best, Brian --- robert burrell donkin [EMAIL PROTECTED] wrote: the consensus seems to be that a 1.0.5 release is a good thing and that people are happy with me acting as release manager. the release plan can be found here: http://wiki.apache.org/jakarta-commons/Logging/1_2e0_2e5ReleasePlan it's good to see that people have already started work (thanks denis :) please feel free to dive in! comments are especially useful for the bugs parade. it'd probably be best for folks to attached their comments in a sub-list below each one. i'll prepare an proposal containing the analysis for the list a little later. i propose to start work on this release pretty much as soon as the repository has been converted to subversion. i'll add replies to this thread to keep people up to date... - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do you Yahoo!? Yahoo! Mail - Helps protect you from nasty viruses. http://promotions.yahoo.com/new_mail - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: logging: WeakHashtable
--- robert burrell donkin [EMAIL PROTECTED] wrote: i've commit the patch of the 15th (with a few minor changes). thanks. Great. Thanks much :) this means that the value is (effectively) held by a hard reference whilst the key is held by a weak. this should address the narrow issue of the most typical use case. i think that probably the code could be improved by eliminating some unnecessary complexity but that'll have to wait till tomorrow. Actually, I think it should work with the commons-logging in WEB-INF/lib case as well. In that use case, the relevant LogFactory and WeakHashtable are actually loaded by the webapp classloader, so they too are gc'ed when the webapp is undeployed. Attached is a patch to LogFactoryTest that I believe shows this. Please let me know if this is wrongheaded in some way -- I wouldn't be surprised at all if it is. I took some time to study what the existing LoadTest class does and tried to mimic it to test different classloading configurations. (In fact, if this patch is accepted down the road I should do a quick refactor to avoid duplication w/ LoadTest). The other day when I thought it wouldn't work in the WEB-INF/lib situation, my test classloader was delegating all loading to its parent EXCEPT LogFactoryImpl. LogFactory and everything else was loaded by the parent. That's not the same as putting commons-lib in WEB-INF/lib. The classloader in the attached patch is a more correct mock of a webapp classloader. My mistake the other day does show one area where WeakHashtable will fail -- if a custom subclass of LogFactory were deployed in WEB-INF/lib but commons-logging.jar were on the server classpath. But I expect that's a pretty small use case. Oh, BTW, the patch also removes the previous test of whether the LogFactory is eventually released once the classloader is released. Now it just tests if the classloader is released. Testing release of the LogFactory basically duplicated what was already done in WeakHashtableTest. one interesting feature of garbage collectors (which foxed me for a while) is that there doesn't seem to be any guarantee as to when the reference is placed onto the queue. (at least i can't find one: if anyone knows different please jump in.) on the (macOSX) JVM i use, it appears that the reference is placed onto the queue late enough to cause one of the tests to fail. placing a hard loop that polls for the released reference to be placed on another queue results in the test passing... Interesting. This whole thing's been interesting -- way more than I thought it would be when I started off grunting through a bunch of reflection :) Cheers, Brian On 16 Nov 2004, at 15:12, Brian Stansberry wrote: Attached is a patch to LogFactory that attempts to play classloader tricks to prevent the hard reference problem. This is not meant to be applied; the patch is just a shorthand way to communicate ideas I'm playing with. P.S. Thanks to all for not laughing at this one! This seemed to work in the sense that the tests I added to LogFactoryTest pass. But, o.a.c.l.LoadTest fails with a ClassCastException. Other tests in that package pass. Haven't had time to try and analyze what's going on with the LoadTest. Gotta run to work. - brian __ Do you Yahoo!? The all-new My Yahoo! - Get yours free! http://my.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do you Yahoo!? The all-new My Yahoo! - Get yours free! http://my.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: logging: WeakHashtable
snip My mistake the other day does show one area where WeakHashtable will fail -- if a custom subclass of LogFactory were deployed in WEB-INF/lib but commons-logging.jar were on the server classpath. But I expect that's a pretty small use case. i suspect this extends to pretty much anyone who uses a custom LogFactory implementation where commons-logging is in a parent classloader and the implementation is deployed in the child. Yep. custom LogFactory implementations are not very useful at the moment and so i'd be happy just to live with a note in the documentation about this limitation. Sounds good. I'll put some thought to a good note, although it might be a few days. Were you thinking in the Javadoc, or somewhere else? in addition, this use case will be addressed very well by the bytecode stuff. (the idea is that instead of discovering a log factory at runtime, all the calls will be rewired when the classes are enhanced.) if you're deploying a stand alone web-app with a definite need to use a particular LogFactory, it's more reliable to dope all the jar's than to rely on discovery. i'll look forward to see your patch (either i've missed it, i'm confused or it was stripped this time...) Don't know what happened. Late night gremlins. When I get home (prob 8 hours from now) I'll attach it to Bugzilla. i'll leave my tidy up for a few days (give you a chance to get patching without me treading on your toes). once everyone's happy with the class, i plan to start pushing towards a 1.0.5 release. it'll probably be release from a branch so that the release candidate for long enough. Great. Thanks for everything. Brian __ Do you Yahoo!? The all-new My Yahoo! - Get yours free! http://my.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: logging: WeakHashtable
Attached is a patch to LogFactory that attempts to play classloader tricks to prevent the hard reference problem. This is not meant to be applied; the patch is just a shorthand way to communicate ideas I'm playing with. This seemed to work in the sense that the tests I added to LogFactoryTest pass. But, o.a.c.l.LoadTest fails with a ClassCastException. Other tests in that package pass. Haven't had time to try and analyze what's going on with the LoadTest. Gotta run to work. - brian __ Do you Yahoo!? The all-new My Yahoo! - Get yours free! http://my.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging]: WeakHashtable
--- robert burrell donkin [EMAIL PROTECTED] wrote: hi brian sorry i haven't got back early (didn't seem to stop all weekend). No worries. It's good to have a real life :) i've taken a first pass through your emails and i'll definitely be drafting replies but i'm not sure how much time i'll get tonight (and i might need to sleep on the code...) Sounds good. Sleep is good Also, one thought in the back of my mind is that a ReferenceQueue also makes it easy to do cleanup in small doses. E.g. every X number of calls to put() or something, pop one key off the queue and remove it. - brian __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: logging: WeakHashtable
--- robert burrell donkin [EMAIL PROTECTED] wrote: On 12 Nov 2004, at 06:55, Brian Stansberry wrote: But then I thought, wait, should the values be held in WeakReferences? In a typical case where the application just calls LogFactory.getLog(), won't the only reference to the LogFactory instance be the value in the map? In this case a lot of calls to getLog() will end up going through the getFactory() discovery mechanism as the GC keeps clearing the values from the hashtable. this is actually quite a big issue. the reason why i made the LogFactory references weakly held was that there's a strong reference from any object to it's classloader (via getClass().getClassloader(). (unless there's some special mojo for this case which i'm unaware of) i'd say that whilst the LogFactory is hard referenced, so is it's classloader. (i should probably create a test to prove this reasoning.) Interesting. The LogFactoryTest.testReleaseFactories() test inadvertently shows a classloader is released. However, in the test, LogFactoryImpl was not loaded by the classloader that creates the map key. I can play with this tonight to see what happens if I change that. but you're absolutely right that using a weak reference to hold the LogFactory may well result in poor performance. i've tried to think my way around this one, by thinking of possible ways of reference the two but (so far) i haven't come up with one. the only alternative approaches i've come up with so way are more than a little ugly, using counters (timers or something) so that references to values are weakened periodically. anyone have any better ideas? I'm at work at the moment, so can't think too much ;-) Would getClass().getClassloader() return the Thread contextClassLoader that was in effect when getFactory() was first called, or a parent classloader if the parent was the one that actually loaded the class? E.g. a situation where a web app classloader first calls getFactory(), but because commons-logging.jar is on the server classpath it is actually loaded by a server classloader. I can test this tonight as well. - brian __ Do you Yahoo!? The all-new My Yahoo! - Get yours free! http://my.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: logging: WeakHashtable
OK, been playing with different classloading
configurations to see if the loader gets stuck in map.
Good new/bad news.
1) As you'd expect,
LogFactoryTest.testReleaseFactories() will definitely
work because the dummy classloader I use in the call
to getFactory() was not the contextClassLoader when
LogFactoryImpl was first loaded. Duh.
2) Fixed that by ensuring that a trivial test
classloader was the thread context loader when
LogFactoryImpl was first loaded. TestClassLoader is
as follows:
private static final Class TestClassLoader
extends ClassLoader {
private TestClassLoader(ClassLoader parent) {
super(parent);
}
}
Again, the test passes. This is because the
TestClassLoader delegates to its parent, so the
LogFactory holds a reference to the parent, not the
child. Good news. I expect this would be a typical
situation with things like EJB classloaders and all
others that follow the Java2 delegation model.
3) Beefed up TestClassLoader by overriding loadClass()
so it was able to load LogFactoryImpl itself and
wouldn't delegate to its parent. This simulates a web
app situation, where the web app classloader ignores
normal Java2 classloading and may find its own copy of
commons-logging in WEB-INF/lib. Bad news. As you
expected, the test fails because the test classloader
is kept in the map by the hard reference from
LogFactoryImpl.
So, it seems like a hard reference in the map to a
LogFactory is mostly a problem for webapps that
include commons-logging in WEB-INF when it is already
available on the server classpath. Bad practice in
general to do this, but people do the darnedest
things. I know Tomcat and the embedded Tomcat in
JBoss both guard against this problem by calling
LogFactory.release() when they undeploy webapps.
Don't know about other webservers.
(BTW, the problem with JBoss that led to Bug 31286 is
related to EJB deployments. commons-logging is on the
JBoss server classpath (specifically in the
UnifiedClassLoaderRepository) due to its use in
embedded Tomcat, so the classloader situation should
be analogous to #2 above. JBoss doesn't call
LogFactory.release() when undeploying EJBs, because if
embedded Tomcat is not deployed they can't be sure
LogFactory will be available).
- brian
__
Do you Yahoo!?
Meet the all-new My Yahoo! - Try it today!
http://my.yahoo.com
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [logging]: WeakHashtable
I've attached a patch that adds a couple more tests and fixes a problem found by one of them. New test LogFactoryTest.testHoldsFactories() shows that WeakHashtable does not keep a LogFactory from being gc'ed even if the ClassLoader associated with it is still alive. So, calls to LogFactory.getFactory() result in new factories being created. The patch to WeakHashtable is largely designed to fix that. The patched WeakHashtable holds values in the table until the WeakReference to the associated ClassLoader is removed, even if the classloader itself has been gc'ed. Because of this, the potential amount of garbage left in the table is greater than it was before. The patch partly tries to remedy that by doing a purge before each rehash. The patch also switches the purge mechanism to one that uses a ReferenceQueue. This should be more performant, as it allows the purging process to only deal with items (if any) that definitely need to be removed from the hashtable, rather than iterating through all entries in the map. ReferenceQueue.poll() itself is quite fast, basically consisting of popping off the first element in a linked list. In this patch the way LogFactories are kept from being dropped from the hashtable is not ideal. Basically the keys in the map hold hard references to the LogFactories, keeping the WeakReferences to the LogFactories from being cleared. This approach is a leftover remnant of a failed attempt on my part at getting the hashtable itself to clear unneeded factories without the need for a call to purge(). It would be much cleaner to just have the hashtable hold normal hard references to the LogFactories. I didn't include such a change in this patch as 1) it may have made the patch overly complicated, and 2) I didn't have time ;-) If the powers that be agree that the LogFactories should be held directly by the Hashtable, I would be happy to create another patch. (Also, there's some funky stuff in the test cases where I try to handle OutOfMemoryError. It works on my environment (Eclipse 3.0, Sun JDK 1.4.2_03), but if others have thoughts about this, they would be much appreciated). Best, Brian --- Brian Stansberry [EMAIL PROTECTED] wrote: --- robert burrell donkin wrote: On 11 Nov 2004, at 07:40, Brian Stansberry wrote: A couple things occurred to me as I looked. 1) The instances of Referenced are not cleared from the underlying table if their underlying references are cleared. 2) Passing null keys/values to put() does not result in a NPE. One thought on #1 is to make Referenced a subclass of WeakReference instead of a wrapper. You can then keep a ReferenceQueue and poll it to remove cleared references from the hashtable whenever get() is called. This is similar to what WeakHashMap does. i had a bit of a think about the best way to do this. i think the approach outlined would be best if this were a general implementation. in this case, though, the majority of calls are going to be to get with somewhat less going to put and very few to any others. i can't think of any occasions when the symantics of put and get are influenced by the presence of extra entries. so i've opted for code that simply purges entries that have lost their referants which is called at the start of other interrogative methods. the data returned will be more stale than using a reference queue but i think that liveliness for put and get should be improved. Yep, slowing down the critical get() just to sweep up some dust in the corners makes no sense. i'd be grateful if people would take a look at the code and try to find any holes in this approach or reasons why using a ReferenceQueue might improve liveliness (preferably with patches)... I was thinking about this and concluded that the approach of iterating the Hashtable.entrySet() would be faster since you're checking if either the key or the value has been cleared. Using a ReferenceQueue for values would force you to use a reverse lookup map, which seems inefficient. But then I thought, wait, should the values be held in WeakReferences? In a typical case where the application just calls LogFactory.getLog(), won't the only reference to the LogFactory instance be the value in the map? In this case a lot of calls to getLog() will end up going through the getFactory() discovery mechanism as the GC keeps clearing the values from the hashtable. Brian __ Do you Yahoo!? Check out the new Yahoo! Front Page. www.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] __ Do you Yahoo!? Check out the new Yahoo! Front Page. www.yahoo.com Index: optional/src
Re: logging: WeakHashtable
--- robert burrell donkin wrote: On 11 Nov 2004, at 07:40, Brian Stansberry wrote: A couple things occurred to me as I looked. 1) The instances of Referenced are not cleared from the underlying table if their underlying references are cleared. 2) Passing null keys/values to put() does not result in a NPE. One thought on #1 is to make Referenced a subclass of WeakReference instead of a wrapper. You can then keep a ReferenceQueue and poll it to remove cleared references from the hashtable whenever get() is called. This is similar to what WeakHashMap does. i had a bit of a think about the best way to do this. i think the approach outlined would be best if this were a general implementation. in this case, though, the majority of calls are going to be to get with somewhat less going to put and very few to any others. i can't think of any occasions when the symantics of put and get are influenced by the presence of extra entries. so i've opted for code that simply purges entries that have lost their referants which is called at the start of other interrogative methods. the data returned will be more stale than using a reference queue but i think that liveliness for put and get should be improved. Yep, slowing down the critical get() just to sweep up some dust in the corners makes no sense. i'd be grateful if people would take a look at the code and try to find any holes in this approach or reasons why using a ReferenceQueue might improve liveliness (preferably with patches)... I was thinking about this and concluded that the approach of iterating the Hashtable.entrySet() would be faster since you're checking if either the key or the value has been cleared. Using a ReferenceQueue for values would force you to use a reverse lookup map, which seems inefficient. But then I thought, wait, should the values be held in WeakReferences? In a typical case where the application just calls LogFactory.getLog(), won't the only reference to the LogFactory instance be the value in the map? In this case a lot of calls to getLog() will end up going through the getFactory() discovery mechanism as the GC keeps clearing the values from the hashtable. Brian __ Do you Yahoo!? Check out the new Yahoo! Front Page. www.yahoo.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
logging: WeakHashtable
Hi Robert,
Had a little time to look at the WeakHashtable. Much cleaner w/o reflection!
A couple things occurred to me as I looked.
1) The instances of Referenced are not cleared from the underlying table if their underlying references are cleared.
2) Passing null keys/values to put() does not result in a NPE.
I attached a patch to the WeakHashtableTest that tests for these.
One thought on #1 is to make Referenced a subclass of WeakReference instead of a wrapper. You can thenkeep a ReferenceQueue and poll it to remove cleared references from the hashtable whenever get() is called. This is similar to what WeakHashMap does.
Best,
Brian Stansberry
Do you Yahoo!?
Check out the new Yahoo! Front Page. www.yahoo.comIndex: WeakHashtableTest.java
===
RCS file:
/home/cvspublic/jakarta-commons/logging/optional/src/test/org/apache/commons/logging/impl/WeakHashtableTest.java,v
retrieving revision 1.1
diff -u -r1.1 WeakHashtableTest.java
--- WeakHashtableTest.java 10 Nov 2004 22:59:54 - 1.1
+++ WeakHashtableTest.java 11 Nov 2004 07:04:14 -
@@ -153,6 +153,24 @@
weakHashtable.put(anotherKey, new Long(1066));
assertEquals(new Long(1066), weakHashtable.get(anotherKey));
+
+// Test compliance with the hashtable API re nulls
+Exception caught = null;
+try {
+weakHashtable.put(null, new Object());
+}
+catch (Exception e) {
+caught = e;
+}
+assertNotNull(did not throw an exception adding a null key, caught);
+caught = null;
+try {
+weakHashtable.put(new Object(), null);
+}
+catch (Exception e) {
+caught = e;
+}
+assertNotNull(did not throw an exception adding a null value,
caught);
}
/** Tests public void putAll(MapÊt) */
@@ -214,5 +232,8 @@
bytz = bytz * 2;
}
}
+
+// Test that the released objects are not taking space in the table
+assertEquals(underlying table not emptied, 0, weakHashtable.size());
}
}
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
