Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-5769

RobustReflectionConverter is not used in some cases

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Component/s: core
    • Labels:
      None

      Description

      There are a few cases where XStream internally passes its default instance of ReflectionConverter to other converter instances. When these converters drill down into their fields, they do not use RobustReflectionConverter and the overall conversion becomes subject to the same failures that RobustReflectionConverter was created to avoid. Here is a list of converters that are passed the default ReflectionConverter in XStream 1.3.1:

      ThrowableConverter
      RegexPatternConverter
      SerializableConverter
      LookAndFeelConverter
      SelfStreamingInstanceChecker

      In our case, the fact that the ThrowableConverter was not calling RobustReflectionConvert combined with an error in class member munging described in JENKINS-5768 to cause our build.xml deserializable to fail, cause the build in question to not be loaded into memory on startup.

        Activity

        Hide
        mdillon mdillon added a comment -

        For what it's worth, I was able to work around this issue for the ThrowableConverter case by putting the following code into a plugin:

        static {
            fixConverters(hudson.model.Hudson.XSTREAM);
            fixConverters(hudson.model.Items.XSTREAM);
            fixConverters(hudson.model.Run.XSTREAM);
        }
        
        private static void fixConverters(XStream xs) {
            Converter converter = xs.getConverterLookup().lookupConverterForType(Object.class);
            if (converter instanceof RobustReflectionConverter) {
                xs.registerConverter(new ThrowableConverter(converter), 10);
            }
        }
        
        Show
        mdillon mdillon added a comment - For what it's worth, I was able to work around this issue for the ThrowableConverter case by putting the following code into a plugin: static { fixConverters(hudson.model.Hudson.XSTREAM); fixConverters(hudson.model.Items.XSTREAM); fixConverters(hudson.model.Run.XSTREAM); } private static void fixConverters(XStream xs) { Converter converter = xs.getConverterLookup().lookupConverterForType( Object .class); if (converter instanceof RobustReflectionConverter) { xs.registerConverter( new ThrowableConverter(converter), 10); } }
        Hide
        mdillon mdillon added a comment -

        I looked at this a little closer and the only converters that have a direct reference to the original ReflectionConverter are ThrowableConverter, RegexPatternConverter, and SelfStreamingInstanceChecker. The other ones I mention take the "reflectionProvider" in the constructor, not the "reflectionConverter".

        Show
        mdillon mdillon added a comment - I looked at this a little closer and the only converters that have a direct reference to the original ReflectionConverter are ThrowableConverter, RegexPatternConverter, and SelfStreamingInstanceChecker. The other ones I mention take the "reflectionProvider" in the constructor, not the "reflectionConverter".
        Hide
        mdillon mdillon added a comment -

        This patch changes XStream2 to "replace" ThrowableConverter, RegexPatternConverter, and SelfStreamingInstanceChecker with ones that delegate to RobustReflectionConverter.

        The only one I've seen fail is ThrowableConverter. Failure of RegexPatternConverter seems unlikely since Pattern is not generally subclasses. I'm not sure what SelfStreamingInstanceChecker actually does, so I can't say whether or not the likelihood of a failure in the default ReflectionConverter is likely to bubble up and cause it to have an exception itself.

        Show
        mdillon mdillon added a comment - This patch changes XStream2 to "replace" ThrowableConverter, RegexPatternConverter, and SelfStreamingInstanceChecker with ones that delegate to RobustReflectionConverter. The only one I've seen fail is ThrowableConverter. Failure of RegexPatternConverter seems unlikely since Pattern is not generally subclasses. I'm not sure what SelfStreamingInstanceChecker actually does, so I can't say whether or not the likelihood of a failure in the default ReflectionConverter is likely to bubble up and cause it to have an exception itself.
        Hide
        mdillon mdillon added a comment -

        One more note. It seems like the right fix for this might be in XStream. It seems like they should be using the convertAnother method on the MarshallingContext/UnmarshallingContext, but those method can't be passed a HierarchicalStreamWriter or HierarchicalStreamReader. Not sure if the contexts have enough information for that not to matter.

        Show
        mdillon mdillon added a comment - One more note. It seems like the right fix for this might be in XStream. It seems like they should be using the convertAnother method on the MarshallingContext/UnmarshallingContext, but those method can't be passed a HierarchicalStreamWriter or HierarchicalStreamReader. Not sure if the contexts have enough information for that not to matter.
        Hide
        mindless Alan Harder added a comment -

        I don't think convertAnother can be used, as it will just come back to ThrowableConverter, creating an infinite loop.. know a way around this?

        Show
        mindless Alan Harder added a comment - I don't think convertAnother can be used, as it will just come back to ThrowableConverter, creating an infinite loop.. know a way around this?
        Hide
        mdillon mdillon added a comment -

        That must be why they maintain the direct reference to the default converter. You're right that using convertAnother would not be workable. An alternative change for XStream that might be helpful would be for them to add a "protected Converter createReflectionConverter()" on the XStream class. That would let XStream2 replace the default reflection converter with RobustReflectionConverter.

        Putting aside the discussion of what could be changed in XStream itself, does my patch for Hudson seem workable? Aside from the nastiness of having to know the priority associated with the original converters and the lack of the ability to simply replace those converters directly, it seems like a workable approach to me.

        Show
        mdillon mdillon added a comment - That must be why they maintain the direct reference to the default converter. You're right that using convertAnother would not be workable. An alternative change for XStream that might be helpful would be for them to add a "protected Converter createReflectionConverter()" on the XStream class. That would let XStream2 replace the default reflection converter with RobustReflectionConverter. Putting aside the discussion of what could be changed in XStream itself, does my patch for Hudson seem workable? Aside from the nastiness of having to know the priority associated with the original converters and the lack of the ability to simply replace those converters directly, it seems like a workable approach to me.
        Hide
        mindless Alan Harder added a comment -

        Yes, both solutions are workable.. we already have a forked XStream, and I'm on the fence about whether to add another custom change in there. But I do like that createReflectionConverter (or maybe createDefaultConverter) idea, as it would make the full registry of converters a bit cleaner.. "replacing" ThrowableConverter, etc. actually leaves the original instances in the registry, though I guess they'd never be used.
        Pattern is a final class, so I'd expect RegexPatternConverter to always work.. SelfStreamingInstanceChecker is for serializing XStream[2] objects which I doubt we'd ever do.. so really only ThrowableConverter matters here, though I guess we can replace them all just for completeness (or modify XStream as you suggested and then they'll all have the right default converter anyway).

        Show
        mindless Alan Harder added a comment - Yes, both solutions are workable.. we already have a forked XStream, and I'm on the fence about whether to add another custom change in there. But I do like that createReflectionConverter (or maybe createDefaultConverter) idea, as it would make the full registry of converters a bit cleaner.. "replacing" ThrowableConverter, etc. actually leaves the original instances in the registry, though I guess they'd never be used. Pattern is a final class, so I'd expect RegexPatternConverter to always work.. SelfStreamingInstanceChecker is for serializing XStream [2] objects which I doubt we'd ever do.. so really only ThrowableConverter matters here, though I guess we can replace them all just for completeness (or modify XStream as you suggested and then they'll all have the right default converter anyway).
        Hide
        mindless Alan Harder added a comment -
        Show
        mindless Alan Harder added a comment - Going with createDefaultConverter(). http://github.com/hudson/xstream/commit/b52438619b90231ce6fc37b4d0053079a98567e4
        Hide
        mdillon mdillon added a comment -

        Looks great.

        Also, FWIW, I came to the same conclusion with respect to RegexPatternConverter and SelfStreamingInstanceChecker.

        Show
        mdillon mdillon added a comment - Looks great. Also, FWIW, I came to the same conclusion with respect to RegexPatternConverter and SelfStreamingInstanceChecker.
        Hide
        mindless Alan Harder added a comment -

        Waiting on Kohsuke to release XStream 1.3.1-hudson-6 so I can commit the fix.

        Show
        mindless Alan Harder added a comment - Waiting on Kohsuke to release XStream 1.3.1-hudson-6 so I can commit the fix.
        Hide
        mdillon mdillon added a comment -

        I noticed in the diff that you changed the pom.xml from 1.3.1-hudson-2 to 1.3.1-hudson-6. Did you happen to check whether the sources in github were otherwise up-to-date with 1.3.1-hudson-5 first?

        Show
        mdillon mdillon added a comment - I noticed in the diff that you changed the pom.xml from 1.3.1-hudson-2 to 1.3.1-hudson-6. Did you happen to check whether the sources in github were otherwise up-to-date with 1.3.1-hudson-5 first?
        Hide
        mindless Alan Harder added a comment -

        Yes, I first committed to the "master" branch and noticed the same thing.. looked around and found the "patched" branch which had -hudson-5. Got the commit merged over there before Kohsuke did the release. Thanks for checking

        Show
        mindless Alan Harder added a comment - Yes, I first committed to the "master" branch and noticed the same thing.. looked around and found the "patched" branch which had -hudson-5. Got the commit merged over there before Kohsuke did the release. Thanks for checking
        Hide
        mindless Alan Harder added a comment -
        r28141 | mindless | 2010-03-02 10:55:42 -0800 (Tue, 02 Mar 2010) | 8 lines
        Changed paths:
           M /trunk/hudson/main/core/pom.xml
           M /trunk/hudson/main/core/src/main/java/hudson/util/XStream2.java
           M /trunk/hudson/main/core/src/test/java/hudson/util/XStream2Test.java
           M /trunk/www/changelog.html
        
        [FIXED JENKINS-5769] Update to XStream 1.3.1-hudson-6 where I added 
        protected Converter createDefaultConverter().  This allows a subclass
        to override the default converter (was hardcoded to ReflectionConverter)
        which is also given to the constructor of a few other converters,
        most notably ThrowableConverter.  Override this method in XStream2 to
        provide a RobustReflectionConverter so missing fields in classes
        extending Throwable are handled properly.
        
        Show
        mindless Alan Harder added a comment - r28141 | mindless | 2010-03-02 10:55:42 -0800 (Tue, 02 Mar 2010) | 8 lines Changed paths: M /trunk/hudson/main/core/pom.xml M /trunk/hudson/main/core/src/main/java/hudson/util/XStream2.java M /trunk/hudson/main/core/src/test/java/hudson/util/XStream2Test.java M /trunk/www/changelog.html [FIXED JENKINS-5769] Update to XStream 1.3.1-hudson-6 where I added protected Converter createDefaultConverter(). This allows a subclass to override the default converter (was hardcoded to ReflectionConverter) which is also given to the constructor of a few other converters, most notably ThrowableConverter. Override this method in XStream2 to provide a RobustReflectionConverter so missing fields in classes extending Throwable are handled properly.
        Hide
        scm_issue_link SCM/JIRA link daemon added a comment -

        Code changed in hudson
        User: : mindless
        Path:
        trunk/hudson/main/core/pom.xml
        trunk/hudson/main/core/src/main/java/hudson/util/XStream2.java
        trunk/hudson/main/core/src/test/java/hudson/util/XStream2Test.java
        trunk/www/changelog.html
        http://jenkins-ci.org/commit/28141
        Log:
        [FIXED JENKINS-5769] Update to XStream 1.3.1-hudson-6 where I added
        protected Converter createDefaultConverter(). This allows a subclass
        to override the default converter (was hardcoded to ReflectionConverter)
        which is also given to the constructor of a few other converters,
        most notably ThrowableConverter. Override this method in XStream2 to
        provide a RobustReflectionConverter so missing fields in classes
        extending Throwable are handled properly.

        Show
        scm_issue_link SCM/JIRA link daemon added a comment - Code changed in hudson User: : mindless Path: trunk/hudson/main/core/pom.xml trunk/hudson/main/core/src/main/java/hudson/util/XStream2.java trunk/hudson/main/core/src/test/java/hudson/util/XStream2Test.java trunk/www/changelog.html http://jenkins-ci.org/commit/28141 Log: [FIXED JENKINS-5769] Update to XStream 1.3.1-hudson-6 where I added protected Converter createDefaultConverter(). This allows a subclass to override the default converter (was hardcoded to ReflectionConverter) which is also given to the constructor of a few other converters, most notably ThrowableConverter. Override this method in XStream2 to provide a RobustReflectionConverter so missing fields in classes extending Throwable are handled properly.

          People

          • Assignee:
            mindless Alan Harder
            Reporter:
            mdillon mdillon
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: