River
  1. River
  2. RIVER-416

The com.sun.jini.logging.Levels class produces a RuntimeException with the latest version of Java

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: River_2.2.0
    • Fix Version/s: River_2.2.1
    • Component/s: com_sun_jini_logging
    • Labels:
      None

      Description

      The com.sun.jini.logging.Levels class produces a RuntimeException with the latest version of Java (both 1.6 and 1.7). The issue surrounds creation of custom java.util.logging.Level. The current implementation uses a ClassReplacingObjectOutputStream and the LevelData approach. By removing this approach and creating a subclass of java.util.logging.Level the issue gets resolved.

      1. Levels.java
        3 kB
        Dennis Reedy
      2. Levels.patch
        7 kB
        Mark Brouwer
      3. Levels.java
        6 kB
        Mark Brouwer

        Issue Links

          Activity

          Hide
          Greg Trasuk added a comment -

          JDK7 doc for java.util.logging.Level says:
          --begin
          It is possible for third parties to define additional logging levels by subclassing Level. In such cases subclasses should take care to chose unique integer level values and to ensure that they maintain the Object uniqueness property across serialization by defining a suitable readResolve method.
          --end

          What do they mean about the Object uniqueness property? And does that mean something else needs to be done in the patch?

          Show
          Greg Trasuk added a comment - JDK7 doc for java.util.logging.Level says: --begin It is possible for third parties to define additional logging levels by subclassing Level. In such cases subclasses should take care to chose unique integer level values and to ensure that they maintain the Object uniqueness property across serialization by defining a suitable readResolve method. --end What do they mean about the Object uniqueness property? And does that mean something else needs to be done in the patch?
          Hide
          Dennis Reedy added a comment -

          Inn looking at Level, a Level is equal to another Level if they have the same integer value. The readResolve() for Level is:

          // Serialization magic to prevent "doppelgangers".
          // This is a peformance optimization.
          private Object readResolve() {
              synchronized (Level.class) {
              for (int i = 0; i < known.size(); i++) {
          	Level other = (Level) known.get(i);
          	if (this.name.equals(other.name) && this.value == other.value
          		&& (this.resourceBundleName == other.resourceBundleName ||
          		    (this.resourceBundleName != null &&
          		    this.resourceBundleName.equals(other.resourceBundleName)))) {
          	    return other;
          	}
              }
              // Woops.  Whoever sent us this object knows 
              // about a new log level.  Add it to our list.
              known.add(this);
              return this;
              }
          }    
          

          I'm not seeing why we need to add a different readResolve()?

          Show
          Dennis Reedy added a comment - Inn looking at Level, a Level is equal to another Level if they have the same integer value. The readResolve() for Level is: // Serialization magic to prevent "doppelgangers" . // This is a peformance optimization. private Object readResolve() { synchronized (Level.class) { for ( int i = 0; i < known.size(); i++) { Level other = (Level) known.get(i); if ( this .name.equals(other.name) && this .value == other.value && ( this .resourceBundleName == other.resourceBundleName || ( this .resourceBundleName != null && this .resourceBundleName.equals(other.resourceBundleName)))) { return other; } } // Woops. Whoever sent us this object knows // about a new log level. Add it to our list. known.add( this ); return this ; } } I'm not seeing why we need to add a different readResolve()?
          Hide
          Greg Trasuk added a comment -

          Looks like this "object uniqueness" criteria is handled in the superclass Level. No objections from me, then. Might want to give it til tomorrow to see if anyone else reviews it.

          Show
          Greg Trasuk added a comment - Looks like this "object uniqueness" criteria is handled in the superclass Level. No objections from me, then. Might want to give it til tomorrow to see if anyone else reviews it.
          Hide
          Dennis Reedy added a comment -

          Sure thing. In the meantime I'll get the branch setup and commit into trunk and the branch.

          Show
          Dennis Reedy added a comment - Sure thing. In the meantime I'll get the branch setup and commit into trunk and the branch.
          Hide
          Hudson added a comment -

          Integrated in River-trunk-jdk7 #145 (See https://builds.apache.org/job/River-trunk-jdk7/145/)
          RIVER-416: Fix com.sun.jini.logging.Levels (Revision 1444164)

          Result = SUCCESS
          dreedy : http://svn.apache.org/viewvc/?view=rev&rev=1444164
          Files :

          • /river/jtsk/trunk/src/com/sun/jini/logging/Levels.java
          Show
          Hudson added a comment - Integrated in River-trunk-jdk7 #145 (See https://builds.apache.org/job/River-trunk-jdk7/145/ ) RIVER-416 : Fix com.sun.jini.logging.Levels (Revision 1444164) Result = SUCCESS dreedy : http://svn.apache.org/viewvc/?view=rev&rev=1444164 Files : /river/jtsk/trunk/src/com/sun/jini/logging/Levels.java
          Hide
          Robert Gibson added a comment -

          Not sure the suggested fix is quite correct, as anybody who serializes the CustomLevel will be unable to deserialize it without the current version of River in the classpath.
          I would suggest something like this instead

              private static Level createLevel(String name,
                                               int value,
                                               String resourceBundleName) {
          
                  new CustomLevel(name, value, resourceBundleName);
                  // Recent Java versions correctly return a Level object instead of the
                  // custom sub-class, but old versions will just give the CustomLevel
                  // back again
                  final Level parse = Level.parse (Integer.toString (value));
          
                  if (parse.getClass ().equals (Level.class)) {
                      return parse;
                  } else {
                      // ... otherwise use previous approach
                  }
              }
          
          Show
          Robert Gibson added a comment - Not sure the suggested fix is quite correct, as anybody who serializes the CustomLevel will be unable to deserialize it without the current version of River in the classpath. I would suggest something like this instead private static Level createLevel( String name, int value, String resourceBundleName) { new CustomLevel(name, value, resourceBundleName); // Recent Java versions correctly return a Level object instead of the // custom sub-class, but old versions will just give the CustomLevel // back again final Level parse = Level.parse ( Integer .toString (value)); if (parse.getClass ().equals (Level.class)) { return parse; } else { // ... otherwise use previous approach } }
          Hide
          Dennis Reedy added a comment -

          An issue with this approach is the is name field never gets properly populated. Perhaps sing reflection to set the name field accessible would work as well.

          final Level parsed = Level.parse(Integer.toString(value));
          if (parsed.getClass().equals (Level.class)) {
              try {
                  Field nameField = parsed.getClass().getField("name");
                  nameField.setAccessible(true);
                  nameField.set(parsed, name);
              } catch (Exception e) {
                  throw new RuntimeException("Unexpected exception", e);
              }
              return parsed;
          } else {
              return new CustomLevel(name, value, resourceBundleName);
          }
          
          Show
          Dennis Reedy added a comment - An issue with this approach is the is name field never gets properly populated. Perhaps sing reflection to set the name field accessible would work as well. final Level parsed = Level.parse( Integer .toString(value)); if (parsed.getClass().equals (Level.class)) { try { Field nameField = parsed.getClass().getField( "name" ); nameField.setAccessible( true ); nameField.set(parsed, name); } catch (Exception e) { throw new RuntimeException( "Unexpected exception" , e); } return parsed; } else { return new CustomLevel(name, value, resourceBundleName); }
          Hide
          Dennis Reedy added a comment -

          I dont actually like either approach. The issue being discussed here is whether a com.sun.jini.logging.Levels.HANDLED or com.sun.jini.logging.Levels.FAILED with the patched Levels class could be loaded be an older version of River. If someone serialized com.sun.jini.logging.Levels, with the inner com.sun.jini.logging.Levels.CustomLevel class, then tried to read it back with an older version of River they will get a CNFE. But is this even a reasonable use-case? After all, com.sun.jini.logging.Levels is in the platform, in jsk-dl.jar (in alot of other places).

          IMO, yes this can happen, but it's an edge case at best. I also dont like the hack of using reflection or the older approach of creating the class descriptor by swapping output streams.

          If we choose to go with the Level.parse() approach, we only get the level integer as the name, not "HANDLED" or "FAILED". Works, but does not meet the usability test by administrators.

          I suggest we move forward with the CustomLevel inner class.

          Show
          Dennis Reedy added a comment - I dont actually like either approach. The issue being discussed here is whether a com.sun.jini.logging.Levels.HANDLED or com.sun.jini.logging.Levels.FAILED with the patched Levels class could be loaded be an older version of River. If someone serialized com.sun.jini.logging.Levels, with the inner com.sun.jini.logging.Levels.CustomLevel class, then tried to read it back with an older version of River they will get a CNFE. But is this even a reasonable use-case? After all, com.sun.jini.logging.Levels is in the platform, in jsk-dl.jar (in alot of other places). IMO, yes this can happen, but it's an edge case at best. I also dont like the hack of using reflection or the older approach of creating the class descriptor by swapping output streams. If we choose to go with the Level.parse() approach, we only get the level integer as the name, not "HANDLED" or "FAILED". Works, but does not meet the usability test by administrators. I suggest we move forward with the CustomLevel inner class.
          Hide
          Robert Gibson added a comment -

          2 small corrections:
          1) The name field does get correctly populated with the suggested approach (as per the Javadoc and my experiments at least, maybe you tried on a buggy JRE?)
          2) I wasn't thinking so much of serialisation/deserialisation by code using different versions of River, but rather deserialisation by code that is River-independent. Currently, logging code can receive LogRecord objects with level == Levels.HANDLED or Levels.FAILED without having a dependence on jsk-dl.jar, since it is just a java.util.logging.Level object. So, introducing the custom subclass would mean that, for example, a log framework which current only has dependencies on java.util would now have to have jsk-dl.jar available to it.

          I think the edge might be wider than you imagine

          Show
          Robert Gibson added a comment - 2 small corrections: 1) The name field does get correctly populated with the suggested approach (as per the Javadoc and my experiments at least, maybe you tried on a buggy JRE?) 2) I wasn't thinking so much of serialisation/deserialisation by code using different versions of River, but rather deserialisation by code that is River-independent. Currently, logging code can receive LogRecord objects with level == Levels.HANDLED or Levels.FAILED without having a dependence on jsk-dl.jar, since it is just a java.util.logging.Level object. So, introducing the custom subclass would mean that, for example, a log framework which current only has dependencies on java.util would now have to have jsk-dl.jar available to it. I think the edge might be wider than you imagine
          Hide
          Dennis Reedy added a comment -

          Thank for the feedback:

          The name field does not get populated. I dont see how it could. If you look at the javadoc states for the return from the Level.parse() method, it states:

          Passing an integer that corresponds to a known name (eg 700) will return the associated name (eg CONFIG). Passing an integer that does not (eg 1) will return a new level name initialized to that value.

          Also, look at the Level.parse() code. When you pass an unknown level, you do not get the name back, you get the Level created with the integer value as its name.

          To your second point; if logging code that is not River based uses River classes (com.sun.jini.logging.Levels), that code needs to either include com.sun.jini.logging classes in it's classpath, or the JVM that is sending that code must have as it's codebase a jar that has the com.sun.jini.logging.Levels class available so the remote JVM can load that custom Level.

          If the former, then I would think that such a system is not River-independant, but rather uses River jars selectively (making them River dependent by definition). If the latter, then they never see jsk-dl.jar directly, it all appears magical with dynamic classloading capabilities

          Show
          Dennis Reedy added a comment - Thank for the feedback: The name field does not get populated. I dont see how it could. If you look at the javadoc states for the return from the Level.parse() method, it states: Passing an integer that corresponds to a known name (eg 700) will return the associated name (eg CONFIG). Passing an integer that does not (eg 1) will return a new level name initialized to that value. Also, look at the Level.parse() code. When you pass an unknown level, you do not get the name back, you get the Level created with the integer value as its name. To your second point; if logging code that is not River based uses River classes (com.sun.jini.logging.Levels), that code needs to either include com.sun.jini.logging classes in it's classpath, or the JVM that is sending that code must have as it's codebase a jar that has the com.sun.jini.logging.Levels class available so the remote JVM can load that custom Level. If the former, then I would think that such a system is not River-independant, but rather uses River jars selectively (making them River dependent by definition). If the latter, then they never see jsk-dl.jar directly, it all appears magical with dynamic classloading capabilities
          Hide
          Robert Gibson added a comment -

          BTW, to get the name field correctly populated you need to first of all make sure the level is known, maybe you skipped a line of code and that is why you weren't seeing it set correctly?

              // Creates a registers as "known" a new Level
              new CustomLevel(name, value, resourceBundleName);
              // Returns a level corresponding to the just-created on (Level or CustomLevel depending on JDK version)
              final Level parse = Level.parse (Integer.toString (value));
          

          It's all a bit of a faff, I don't understand why Sun didn't just give us a constructor ...

          Show
          Robert Gibson added a comment - BTW, to get the name field correctly populated you need to first of all make sure the level is known, maybe you skipped a line of code and that is why you weren't seeing it set correctly? // Creates a registers as "known" a new Level new CustomLevel(name, value, resourceBundleName); // Returns a level corresponding to the just-created on (Level or CustomLevel depending on JDK version) final Level parse = Level.parse ( Integer .toString (value)); It's all a bit of a faff, I don't understand why Sun didn't just give us a constructor ...
          Hide
          Robert Gibson added a comment -

          My contention is that code can receive Levels.HANDLED or Levels.FAILED without having a dependency on com.sun.jini.logging.

          It's true that the code which does

              logger.log(Levels.HANDLED, "Don't worry, I handled it")
          

          does have a dependency on Levels. But the logging framework that actually handles it doesn't (currently).

           --------------            -----------------
          | Jini process | - - - -  | Logging process |
           --------------            -----------------
          
          

          Any time you write an out-of-process LogHandler this is what you do. Maybe I'm not being very clear? If you don't believe me, just serialise Levels.HANDLED to a file and then try and read it back without jsk-dl.jar available. You can currently do it without any error.

          Show
          Robert Gibson added a comment - My contention is that code can receive Levels.HANDLED or Levels.FAILED without having a dependency on com.sun.jini.logging. It's true that the code which does logger.log(Levels.HANDLED, "Don't worry, I handled it" ) does have a dependency on Levels. But the logging framework that actually handles it doesn't (currently). -------------- ----------------- | Jini process | - - - - | Logging process | -------------- ----------------- Any time you write an out-of-process LogHandler this is what you do. Maybe I'm not being very clear? If you don't believe me, just serialise Levels.HANDLED to a file and then try and read it back without jsk-dl.jar available. You can currently do it without any error.
          Hide
          Dennis Reedy added a comment -

          Okay, on the name vs level number, creating the new CustomLevel first does work. I indeed did miss that line, thanks.

          However, reading the LogRecord back in results in a CNFE if the JVM does not have either jsk-platform.jar, or the serialized LogRecord is not annotated with an available codebase that has com.sun.jini.logging.Levels$CustomLevel. So in your scenario above, the Logging process would have a dependency on River.

          Now, if CustomLevel is not created first (or even exists), and a new Level is created (using Level.parse()) that has as its level and name 550 and 600 (respectively HANDLED and FAILED), this all works fine. What we get is:

          logger.log(Levels.HANDLED, "Don't worry, I handled it")
          

          Emits:

          550: Don't worry, I handled it
          

          So, IMO these are the options:

          1. Accept a new runtime dependency for River to get Levels.HANDLED and Levels.FAILED working with names & level numbers
          2. Live with 550 and 600
          3. Continue looking into creating a real Level that does not require a runtime dependency
          Show
          Dennis Reedy added a comment - Okay, on the name vs level number, creating the new CustomLevel first does work. I indeed did miss that line, thanks. However, reading the LogRecord back in results in a CNFE if the JVM does not have either jsk-platform.jar, or the serialized LogRecord is not annotated with an available codebase that has com.sun.jini.logging.Levels$CustomLevel. So in your scenario above, the Logging process would have a dependency on River. Now, if CustomLevel is not created first (or even exists), and a new Level is created (using Level.parse()) that has as its level and name 550 and 600 (respectively HANDLED and FAILED), this all works fine. What we get is: logger.log(Levels.HANDLED, "Don't worry, I handled it" ) Emits: 550: Don't worry, I handled it So, IMO these are the options: Accept a new runtime dependency for River to get Levels.HANDLED and Levels.FAILED working with names & level numbers Live with 550 and 600 Continue looking into creating a real Level that does not require a runtime dependency
          Hide
          Robert Gibson added a comment -

          Strange, my tests with JDK >= 1.6.0_39 and 1.7.0_13 showed that Level.parse() always returned an instance of exactly j.u.l.Level, never a sub-class, are you seeing something different?

          If recent JDKs (which are the only ones to exhibit this bug) do indeed always return a Level, not a CustomLevel, then I maintain my suggestion from my first comment for a hybrid approach.

          Show
          Robert Gibson added a comment - Strange, my tests with JDK >= 1.6.0_39 and 1.7.0_13 showed that Level.parse() always returned an instance of exactly j.u.l.Level, never a sub-class, are you seeing something different? If recent JDKs (which are the only ones to exhibit this bug) do indeed always return a Level, not a CustomLevel, then I maintain my suggestion from my first comment for a hybrid approach.
          Hide
          Dennis Reedy added a comment - - edited

          I just recalled something that Mark Brouwer wrote to the list wrt this issue. In looking at the new Level class as part of this distribution a new field localizedLevelName was added.

          I modified the com.sun.jini.logging.Levels$LevelData class to include this property and bingo, seems to work.

          private static final class LevelData implements Serializable {
              private static final long serialVersionUID = -8176160795706313070L;
              private final String name;
              private final int value;
              private final String resourceBundleName;
              private final String localizedLevelName;
          
              LevelData(String name, int value, String resourceBundleName) {
                  this.name = name;
                  this.value = value;
                  this.resourceBundleName = resourceBundleName;
                  this.localizedLevelName = resourceBundleName == null ? name : null;
              }
          }
          

          Thanks for your feedback on this issue, with your input the best solution was achieved.

          Show
          Dennis Reedy added a comment - - edited I just recalled something that Mark Brouwer wrote to the list wrt this issue. In looking at the new Level class as part of this distribution a new field localizedLevelName was added. I modified the com.sun.jini.logging.Levels$LevelData class to include this property and bingo, seems to work. private static final class LevelData implements Serializable { private static final long serialVersionUID = -8176160795706313070L; private final String name; private final int value; private final String resourceBundleName; private final String localizedLevelName; LevelData( String name, int value, String resourceBundleName) { this .name = name; this .value = value; this .resourceBundleName = resourceBundleName; this .localizedLevelName = resourceBundleName == null ? name : null ; } } Thanks for your feedback on this issue, with your input the best solution was achieved.
          Hide
          Hudson added a comment -

          Integrated in River-trunk-jdk7 #147 (See https://builds.apache.org/job/River-trunk-jdk7/147/)
          RIVER-416: Backed out use of CustomLevel and added 'localizedLevelName' in the com.sun.jini.logging.Levels class (Revision 1449248)

          Result = SUCCESS
          dreedy : http://svn.apache.org/viewvc/?view=rev&rev=1449248
          Files :

          • /river/jtsk/trunk/src/com/sun/jini/logging/Levels.java
          Show
          Hudson added a comment - Integrated in River-trunk-jdk7 #147 (See https://builds.apache.org/job/River-trunk-jdk7/147/ ) RIVER-416 : Backed out use of CustomLevel and added 'localizedLevelName' in the com.sun.jini.logging.Levels class (Revision 1449248) Result = SUCCESS dreedy : http://svn.apache.org/viewvc/?view=rev&rev=1449248 Files : /river/jtsk/trunk/src/com/sun/jini/logging/Levels.java
          Hide
          Robert Gibson added a comment -

          Great, I suppose it works on older JDK versions as well.

          For the record, I still think that a version using only public APIs is better/less fragile across JDK suppliers or versions, using the serialisation trick only as a fallback if the desired result isn't achieved:

             private static Level createLevel(String name,
                                               int value,
                                               String resourceBundleName) {
          
                  new CustomLevel(name, value, resourceBundleName);
                  final Level level = Level.parse (Integer.toString (value));
          
                  if (level.getClass ().equals (Level.class)) {
                      return level;
                  } else {
                      ...
                  }
          

          But the current commit solves my immediate problem so I'm not complaining!

          Show
          Robert Gibson added a comment - Great, I suppose it works on older JDK versions as well. For the record, I still think that a version using only public APIs is better/less fragile across JDK suppliers or versions, using the serialisation trick only as a fallback if the desired result isn't achieved: private static Level createLevel( String name, int value, String resourceBundleName) { new CustomLevel(name, value, resourceBundleName); final Level level = Level.parse ( Integer .toString (value)); if (level.getClass ().equals (Level.class)) { return level; } else { ... } But the current commit solves my immediate problem so I'm not complaining!
          Hide
          Mark Brouwer added a comment -

          Attached a version that can handle the cases when another field is added in the future.

          Show
          Mark Brouwer added a comment - Attached a version that can handle the cases when another field is added in the future.
          Hide
          Hudson added a comment -

          Integrated in River-trunk-jdk7 #148 (See https://builds.apache.org/job/River-trunk-jdk7/148/)
          RIVER-416: Improved robustness of Levels serialization (Revision 1455692)

          Result = ABORTED
          dreedy : http://svn.apache.org/viewvc/?view=rev&rev=1455692
          Files :

          • /river/jtsk/trunk/src/com/sun/jini/logging/Levels.java
          Show
          Hudson added a comment - Integrated in River-trunk-jdk7 #148 (See https://builds.apache.org/job/River-trunk-jdk7/148/ ) RIVER-416 : Improved robustness of Levels serialization (Revision 1455692) Result = ABORTED dreedy : http://svn.apache.org/viewvc/?view=rev&rev=1455692 Files : /river/jtsk/trunk/src/com/sun/jini/logging/Levels.java

            People

            • Assignee:
              Unassigned
              Reporter:
              Dennis Reedy
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development