A malicious app could do
for (int i=0; i < 100000; ++i) {
new Level(“Level” + i, 1000 + i){};
}
Sure idiots can do lots of bad things but I don’t think Levels should be quite
that flexible.
Ralph
On Jan 25, 2014, at 9:39 PM, Remko Popma <[email protected]> wrote:
> I don't think client code can do new Level(){} as the constructor requires
> String and int arguments.
>
> By the way, I am unclear on what went wrong with the enum approach you
> originally took.
> You said:
> StdLevel isn’t a Level because it can’t extend it if it is an enum, so I
> can’t initialize the levels using that.
>
> I don't understand. StdLevel implements the Level interface, right? So what
> do you mean by "it can't extend it"?
>
> The reason I ask is that for the code generation, "real" java enums may be
> easier to deal with than the extensible enum approach.
>
>
> On Sun, Jan 26, 2014 at 2:30 PM, Ralph Goers <[email protected]>
> wrote:
> Out of curiosity, what exactly is the benefit of declaring the class abstract
> when it has a protected constructor? It seems like all you are accomplishing
> is making the instantiation syntax uglier. It also bothers me that open code
> can just do a new Level(){} - which will do nothing but cause problems. I’m
> beginning to think that we should require an annotation on the Level
> declaration of the extension class to avoid that.
>
> Ralph
>
> On Jan 25, 2014, at 9:13 PM, Remko Popma <[email protected]> wrote:
>
>> Ralph,
>> I copied Nick's code _as is_ and had no compile errors.
>> The class is abstract, but instances are defined in the static block as:
>> OFF = new Level("OFF", 0) {}; // note the {}: this creates an anonymous
>> concrete subclass
>>
>> I agree that read access needs to be synchronized as well, not just write
>> access (the constructor).
>> I experimented with several options:
>> * synchronizing on plain Object in both constructor and when accessing the
>> static Map(s)
>> * a ReentrantReadWriteLock
>> * a lock-free implementation
>>
>> I decided against ReentrantReadWriteLock as it has more overhead than plain
>> synchronized access and the write access (in the constructors) is going to
>> be extremely rare: not worth paying the overhead in the more common reads.
>> It is also cumbersome to code.
>>
>> The lock-free implementation uses an AtomicInteger for the ordinals, and an
>> AtomicReference for the Map<String, Level>.
>> In the constructor, create a new Map<String, Level> instance based on the
>> old copy, add the new instance, and try to call compareAndSet to replace the
>> old instance with the new instance. Retry on failure.
>>
>> Finally, simply synchronizing on the constructorLock object in the
>> Level.toLevel() and Level.values() methods may be simplest.
>>
>> Which of the last two is best depends on how often the toLevel() and
>> values() levels are called.
>> It turns out they are only called during reconfiguration, so no real need to
>> optimize these methods.
>> I would argue that simple synchronization may be best in this case.
>>
>> Remko
>>
>>
>>
>> On Sun, Jan 26, 2014 at 1:49 PM, Ralph Goers <[email protected]>
>> wrote:
>> As I am working on this I just want to point out a number of issues with the
>> code below:
>>
>> 1. The class is abstract. The static block is doing a bunch of new Level()
>> invocations which obviously generate compile errors on an abstract class. I
>> had to make it be a non-abstract class.
>> 2. As I pointed out before there is no way to access the “standard” levels
>> as an enum. I have addressed that.
>> 3. Although the constructor is synchronized access to the Map is not. Trying
>> to get from the map while a Level is being added will result in a
>> ConcurrentModificationException. I am using a ConcurrentMap instead.
>> 3. The constructor requires synchronization because it is modifying both the
>> map and the ordinal. However, since this isn’t an enum the ordinal value is
>> of dubious value. Removing that would allow the removal of the
>> synchronization in the constructor. I am considering that but I haven’t done
>> it yet.
>> 4. Your example of creating the extension shows doing a new Level(). This
>> doesn’t work because a) the class is abstract and b) the constructor is
>> protected. I am leaving the constructor protected so extension will require
>> doing new ExtendedLevel(name, value) and creating a constructor. Not
>> requiring that means applications can do a new Level() anywhere and I am
>> opposed to allowing that.
>>
>> Ralph
>>
>> On Jan 23, 2014, at 12:42 AM, Nick Williams <[email protected]>
>> wrote:
>>
>> > Okay, I finally got a minute to read all of these emails, and...
>> >
>> > EVERYBODY FREEZE!
>> >
>> > What if I could get you an extensible enum that required no interface
>> > changes and no binary-incompatible changes at all? Sound too good to be
>> > true? I proposed this months ago (LOG4J2-41) and it got shot down multiple
>> > times, but as of now I've heard THREE people say "extensible enum" in this
>> > thread, so here it is, an extensible enum:
>> >
>> > public abstract class Level implements Comparable<Level>, Serializable {
>> > public static final Level OFF;
>> > public static final Level FATAL;
>> > public static final Level ERROR;
>> > public static final Level WARN;
>> > public static final Level INFO;
>> > public static final Level DEBUG;
>> > public static final Level TRACE;
>> > public static final Level ALL;
>> >
>> >
>> > private static final long serialVersionUID = 0L;
>> > private static final Hashtable<String, Level> map;
>> > private static final TreeMap<Integer, Level> values;
>> > private static final Object constructorLock;
>> >
>> >
>> > static {
>> > // static variables must be constructed in certain order
>> > constructorLock = new Object();
>> > map = new Hashtable<String, Level>();
>> > values = new TreeMap<Integer, Level>();
>> > OFF = new Level("OFF", 0) {};
>> > FATAL = new Level("FATAL", 100) {};
>> > ERROR = new Level("ERROR", 200) {};
>> > WARN = new Level("WARN", 300) {};
>> > INFO = new Level("INFO", 400) {};
>> > DEBUG = new Level("DEBUG", 500) {};
>> > TRACE = new Level("TRACE", 600) {};
>> > ALL = new Level("ALL", Integer.MAX_VALUE) {};
>> > }
>> >
>> >
>> > private static int ordinals;
>> >
>> >
>> > private final String name;
>> > private final int intLevel;
>> > private final int ordinal;
>> >
>> >
>> > protected Level(String name, int intLevel) {
>> > if(name == null || name.length() == 0)
>> > throw new IllegalArgumentException("Illegal null Level
>> > constant");
>> > if(intLevel < 0)
>> > throw new IllegalArgumentException("Illegal Level int less than
>> > zero.");
>> > synchronized (Level.constructorLock) {
>> > if(Level.map.containsKey(name.toUpperCase()))
>> > throw new IllegalArgumentException("Duplicate Level
>> > constant [" + name + "].");
>> > if(Level.values.containsKey(intLevel))
>> > throw new IllegalArgumentException("Duplicate Level int ["
>> > + intLevel + "].");
>> > this.name = name;
>> > this.intLevel = intLevel;
>> > this.ordinal = Level.ordinals++;
>> > Level.map.put(name.toUpperCase(), this);
>> > Level.values.put(intLevel, this);
>> > }
>> > }
>> >
>> >
>> > public int intLevel() {
>> > return this.intLevel;
>> > }
>> >
>> >
>> > public boolean isAtLeastAsSpecificAs(final Level level) {
>> > return this.intLevel <= level.intLevel;
>> > }
>> >
>> >
>> > public boolean isAtLeastAsSpecificAs(final int level) {
>> > return this.intLevel <= level;
>> > }
>> >
>> >
>> > public boolean lessOrEqual(final Level level) {
>> > return this.intLevel <= level.intLevel;
>> > }
>> >
>> >
>> > public boolean lessOrEqual(final int level) {
>> > return this.intLevel <= level;
>> > }
>> >
>> >
>> > @Override
>> > @SuppressWarnings("CloneDoesntCallSuperClone")
>> > public Level clone() throws CloneNotSupportedException {
>> > throw new CloneNotSupportedException();
>> > }
>> >
>> >
>> > @Override
>> > public int compareTo(Level other) {
>> > return intLevel < other.intLevel ? -1 : (intLevel > other.intLevel
>> > ? 1 : 0);
>> > }
>> >
>> >
>> > @Override
>> > public boolean equals(Object other) {
>> > return other instanceof Level && other == this;
>> > }
>> >
>> >
>> > public Class<Level> getDeclaringClass() {
>> > return Level.class;
>> > }
>> >
>> >
>> > @Override
>> > public int hashCode() {
>> > return this.name.hashCode();
>> > }
>> >
>> >
>> > public String name() {
>> > return this.name;
>> > }
>> >
>> >
>> > public int ordinal() {
>> > return this.ordinal;
>> > }
>> >
>> >
>> > @Override
>> > public String toString() {
>> > return this.name;
>> > }
>> >
>> >
>> > public static Level toLevel(String name) {
>> > return Level.toLevel(name, Level.DEBUG);
>> > }
>> >
>> >
>> > public static Level toLevel(String name, Level defaultLevel) {
>> > if(name == null)
>> > return defaultLevel;
>> > name = name.toUpperCase();
>> > if(Level.map.containsKey(name))
>> > return Level.map.get(name);
>> > return defaultLevel;
>> > }
>> >
>> >
>> > public static Level[] values() {
>> > return Level.values.values().toArray(new
>> > Level[Level.values.size()]);
>> > }
>> >
>> >
>> > public static Level valueOf(String name) {
>> > if(name == null)
>> > throw new IllegalArgumentException("Unknown level constant [" +
>> > name + "].");
>> > name = name.toUpperCase();
>> > if(Level.map.containsKey(name))
>> > return Level.map.get(name);
>> > throw new IllegalArgumentException("Unknown level constant [" +
>> > name + "].");
>> > }
>> >
>> >
>> > public static <T extends Enum<T>> T valueOf(Class<T> enumType, String
>> > name) {
>> > return Enum.valueOf(enumType, name);
>> > }
>> >
>> >
>> > // for deserialization
>> > protected final Object readResolve() throws ObjectStreamException {
>> > return Level.valueOf(this.name);
>> > }
>> > }
>> >
>> > Extending it is easy:
>> >
>> > public final class ExtendedLevels {
>> > public static final Level MY_LEVEL = new Level("MY_LEVEL", 250) {};
>> > }
>> >
>> > I still and have ALWAYS believed this was the best option. If we used this
>> > option, I would be fine with not adding any new Levels because I could add
>> > them myself.
>> >
>> > Nick
>> >
>> > On Jan 22, 2014, at 7:04 PM, Remko Popma wrote:
>> >
>> >> This is only a problem for webapps, right?
>> >> Putting log4j jars in WEB-INF/lib avoids that problem (different class
>> >> loader).
>> >> Apps that really want to share log4j jars with other apps would need to
>> >> play nice. Such apps would do well to use a naming convention like Gary
>> >> suggests.
>> >> Otherwise, the last to register would overwrite any previous level with
>> >> the same name. (Should probably emit a StatusLogger warning.)
>> >>
>> >> Same intLevel for different names should not be a problem.
>> >>
>> >>
>> >> On Thursday, January 23, 2014, Gary Gregory <[email protected]>
>> >> wrote:
>> >> Playing devils advocate:
>> >>
>> >> What happens when different apps register levels with the same name and
>> >> different intLevels?
>> >> What happens when different apps register levels with the same intLevel
>> >> and different names?
>> >> Should there be a convention that custom level names be FQNs?
>> >>
>> >> Gary
>> >>
>> >>
>> >> On Wed, Jan 22, 2014 at 10:05 PM, Paul Benedict <[email protected]>
>> >> wrote:
>> >> As Gary wanted, a new thread....
>> >>
>> >> First, each enum needs an inherit strength. This would be part of the
>> >> interface. Forgive me if the word "strength" is wrong; but it's the 100,
>> >> 200, 300, etc. number that triggers the log level. So make sure the
>> >> interface contains the intLevel() method.
>> >>
>> >> Second, we need to know the name, right? The name probably requires a new
>> >> method since it can't be extracted from the enum anymore.
>> >>
>> >> public interface Level {
>> >> int intLevel();
>> >> String name();
>> >> }
>> >>
>> >> PS: The intStrength() name seems hackish. What about strength() or
>> >> treshold()?
>> >>
>> >> Third, the registration can be done manually by providing a static method
>> >> (as your did Remko) that the client needs to invoke, or you could have a
>> >> class-path scanning mechanism. For the latter, you could introduce a new
>> >> annotation to be placed on the enum class.
>> >>
>> >> @CustomLevels
>> >> public enum MyCustomEnums {
>> >> }
>> >>
>> >> Paul
>> >>
>> >> On Wed, Jan 22, 2014 at 8:52 PM, Remko Popma <[email protected]>
>> >> wrote:
>> >> Paul, can you give a bit more detail?
>> >>
>> >> I tried this: copy the current Level enum to a new enum called "Levels"
>> >> in the same package (other name would be fine too). Then change Level to
>> >> an interface (removing the constants and static methods, keeping only the
>> >> non-static methods). Finally make the Levels enum implement the Level
>> >> interface.
>> >>
>> >> After this, we need to do a find+replace for the references to
>> >> Level.CONSTANT to Levels.CONSTANT and Level.staticMethod() to
>> >> Levels.staticMethod().
>> >>
>> >> Finally, the interesting part: how do users add or register their custom
>> >> levels and how do we enable the Levels.staticLookupMethod(String, Level)
>> >> to recognize these custom levels?
>> >>
>> >>
>> >>
>> >> On Thursday, January 23, 2014, Paul Benedict <[email protected]> wrote:
>> >> Agreed. This is not an engineering per se, but really more about if the
>> >> feature set makes sense.
>> >>
>> >> Well if you guys ever look into the interface idea, you'll give log4j the
>> >> feature of getting enums to represent custom levels. That's pretty cool,
>> >> IMO. I don't know if any other logging framework has that and that would
>> >> probably get some positive attention. It shouldn't be so hard to do a
>> >> find+replace on the code that accepts Level and replace it with another
>> >> name. Yes, there will be some minor refactoring that goes with it, but
>> >> hard? It shouldn't be.
>> >>
>> >> A name I propose for the interface is LevelDefinition.
>> >>
>> >> Paul
>> >>
>> >>
>> >> On Wed, Jan 22, 2014 at 6:48 PM, Gary Gregory <[email protected]>
>> >> wrote:
>> >> Hi, I do not see this as an engineering problem but more a feature set
>> >> definition issue. So while there may be lots of more or less internally
>> >> complicated ways of solving this with interfaces, makers and whatnots,
>> >> the built in levels are the most user friendly.
>> >>
>> >> I have have lots of buttons, knobs and settings on my sound system that I
>> >> do not use, just like I do not use all the methods in all the classes in
>> >> the JRE...
>> >>
>> >> Gary
>> >>
>> >>
>> >>
>> >>
>> >> --
>> >> E-Mail: [email protected] | [email protected]
>> >> Java Persistence with Hibernate, Second Edition
>> >> JUnit in Action, Second Edition
>> >> Spring Batch in Action
>> >> Blog: http://garygregory.wordpress.com
>> >> Home: http://garygregory.com/
>> >> Tweet! http://twitter.com/GaryGregory
>> >
>> >
>> > ---------------------------------------------------------------------
>> > 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]
>>
>>
>
>
