Hi Ceki,
See comments inline below...
At 03:05 PM 12/13/2002 +0100, you wrote:
Many thanks for these explanations Jacob. Here are some comments. I
have included a modified version of Log4jCRS.
At 00:10 13.12.2002 -0600, Jacob Kjome wrote:
Here is how it works...
Each application, at startup, configures Log4j in the init() method of a servlet or in the contextInitialized() method of a servlet context listener (the latter being the best choice). Before any call to any configure() method, the following gets run...
Log4jCRS crs = new w();
crs.initLoggerRepository();
How about just calling Log4jCRS.doIdempotentInitialization() instead?
See also below.
Yep, this seems to work great! I've tested the code you attached here and it works flawlessly, except....
I see one possible issue with this. Mark wanted a Log4jInit servlet which was flexible enough to allow configuration of which repository selector to use. However, the first application installed on the server which uses Log4j and a custom repository selector will force that RepositorySelector on all the rest. That might be a bit confusing. Think about a server which serves lots of webapps with a few using Log4j and, of those, a few attempting to use a custom repository selectors. However, a couple apps want to use Log4jCRS and the other couple want to use Log4jJNDI. The first one installed will set its preferred selector and the others will be stuck with that decision no matter which one they preferred. Also, the doIdempotentInitialization() will crap out on subsequent calls to custom repository selectors that weren't the lucky one to get set as the active repository selector. Consider this....
public static void doIdempotentInitialization() {
if(!initialized) {
Object guard = new Object();
LogManager.setRepositorySelector(singleton, guard);
initialized = true;
}
}
If Log4jCRS was the one set as the repository selector, subsequent calls to the Log4jCRS.doIdempotentInitialization() will just skip setting the repository selector with the guard because "initialized" will have already been set to true. However, calls to Log4jJNDI.doIdempotentInitialization() will crap out because "initialized" will still be false so an attempt to set the repository selector will happen, but being that the guard is different than was already set, the set will fail with an "IllegalArgumentException". Unless that code is wrapped with a try/catch, the method will bomb making lots of people unhappy and confused as to why things aren't working.
I guess this is where having the container set the repository selector upon container startup might be applicable.
So, I think we need to discuss how valuable having a Log4jInit servlet that chooses its own repository selector is since it is only a choice for the first app. Secondly, the Selector.doIdempotentInitialization() is aptly named *only* in the case where there is only one possible custom repository selector that could be called. Calls to various other selectors' doIdempotentInitialization() are not idempotent with respect to each's call to LogManager.setRepositorySelector(singleton, guard). Another name might be in order to better describe what that method does...and maybe it should be in the RepositorySelector interface? Thirdly, if nothing else, that method should do the following for safety....
public static void doIdempotentInitialization() {
if(!initialized) {
try {
Object guard = new Object();
LogManager.setRepositorySelector(singleton, guard);
initialized = true;
} catch (IllegalArgumentException iae) {
//either ignore the exception or log the fact that the setting of this
//custom repository selector failed because another had been set previously
// and maybe we should set "initialized" to "true" in here so this exception doesn't
// occur again in this class
}
}
}
Using the contextDestroyed() method of a servlet context listener, one can do the following....
ClassLoader cl = Thread.currentThread().getContextClassLoader();
Log4jCRS crs = Log4jCRS.getCRS(cl);
crs.w
crs.remove(cl);
Wouldn't the following accomplish the same?
LogManager.shutdown();
Log4jCRS crs = Log4jCRS.getSingleton();
crs.remove(cl);
yep, it does. I was under the impression that LogManager.shutdown() would shut down *all* logger repositories. However, I've tested it an it only seems to shut down the current repository. Plus, using the singleton like you are here simplifies things because I don't have to check for null on the Log4jCRS instance as I was before. Nice.
However, there is still one problem. Since various repository selectors might be used here, I can't really count on calling Log4jCRS.getSingleton() because I don't know if Log4jCRS is the current repository selector being used (per comments above). This might be solved by adding getRepositorySelector() to the LogManager interface and remove() to the RepositorySelector interface. Then I could call LogManager.getRepositorySelector().remove(). Instead of taking a ClassLoader, remove() would get the current thread context classloader or do whatever else might be needed by a different repository selector. We could also get rid of getSingleton() from any given selector this way. However, I'm guessing that a change to the LogManager interface is a pretty big deal which I'm not sure would fly. Alternatively, since the Map storing the hierarchies is a WeakHashMap, maybe remove() is unnecessary anyway? Thoughts?
Well put but I still don't understand why you need multiple CRS
instances especially since there can be one and only one active
RepositorySelector? Doesn't the Log4jCRS.getSingleton trick do it?
Yes it does.
Attached a proposed modified version of Log4jCRS based on your changes, now named ContextClassLoaderSelector. It addresses the issues above and requires no changes to any interfaces as long as we assume that no call is needed to remove(). If it is decided that getRepositorySelector() will be added to the LogManager interface and that remove() will be added to the RepositorySelector interface, then we can bring back remove(). The one exception is doIdempotentInitialization(). What we could do, as an alternative to adding this method to the RepositorySelector interface, is maybe create an abstract class called AbstractRepositorySelector and implement a standard way to do the idempotentInitialization(). If someone really wants to override it, they can, but a standard "safe" implementation would be there that they could extend. I'll leave that for continued discussion.
In this new version, all that would happen at startup (in Log4jInit or a servlet context listener) is:
ContextClassLoaderSelector.doIdempotentInitialization();
And all that would happen at shutdown (in a servlet context listener) is:
LogManager.shutdown();
This simplifies things significantly, although a few issues still require working out.
Jake
/* * Copyright (C) The Apache Software Foundation. All rights reserved. * * This software is published under the terms of the Apache Software * License version 1.1, a copy of which has been included with this * distribution in the LICENSE.txt file. */
package org.apache.log4j.selectors;
import org.apache.log4j.spi.RepositorySelector;
import org.apache.log4j.spi.LoggerRepository;
import org.apache.log4j.spi.RootCategory;
import org.apache.log4j.Hierarchy;
import org.apache.log4j.Level;
import org.apache.log4j.LogManager;
import java.util.Collections;
import java.util.Map;
import java.util.WeakHashMap;
/**
* @author Jacob Kjome
*/
public class ContextClassLoaderSelector implements RepositorySelector {
// key: current thread's ContextClassLoader,
// value: Hierarchy instance
final private static Map hierMap = Collections.synchronizedMap(new WeakHashMap());
final private static Log4jCRS singleton = new Log4jCRS();
private static boolean initialized = false;
private Log4jCRS() {}
public LoggerRepository getLoggerRepository() {
ClassLoader cl = Thread.currentThread().getContextClassLoader();
Hierarchy hierarchy = (Hierarchy) hierMap.get(cl);
if(hierarchy == null) {
hierarchy = new Hierarchy(new RootCategory((Level) Level.DEBUG));
hierMap.put(cl, hierarchy);
}
return hierarchy;
}
/**
* The Container should initialize the logger repository for each
* webapp upon startup or reload. In this case, it is controllable
* via each webapp.
*/
public static void doIdempotentInitialization() {
if(!initialized) {
Object guard = new Object();
LogManager.setRepositorySelector(singleton, guard);
initialized = true;
}
}
}
-- To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]> For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
