groovy
  1. groovy
  2. GROOVY-5030

Calling a method overwritten via metaClass from another method uses the original (non-overwritten) method if the overridden class extends something

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2
    • Fix Version/s: 1.8.3, 1.9-beta-4
    • Component/s: primtive opts
    • Labels:
      None
    • Testcase included:
      yes
    • Number of attachments :
      0

      Description

      Hi Everyone

      I'm using Groovy 1.8.2, and there's a variant of http://jira.codehaus.org/browse/GROOVY-4884. This code fails for me:

      import org.junit.Test;
      
      public class BreakingExample_NoMetaclassOverride {
         @Test
         void testNotOverriden() {
            def list = []
            ClassUnderTest cut = new ClassUnderTest()
            cut.metaClass.getRemoteObject = { ->
               return [method: {obj -> 
                  list << obj
               }] as RemoteObject
            }
            
            String val = "Value" 
            cut.someMethod(val)
            assert list == [val]
         }
      }
      
      public class ClassUnderTest extends RemoteObject {
         public def someMethod(String someValue) {
            RemoteObject object = getRemoteObject()
            object.method(someValue)
         }
         protected RemoteObject getRemoteObject() {
            return new RemoteObject()
         }
      }
      
      public class RemoteObject {
         public void method(obj) { /* Something */ }
      }
      

      The only difference between this and #4884 is the "extends RemoteObject" added to ClassUnderTest. With it in, the test fails. Remove it, the test passes.

      Please let me know if you need any more info!

      Thanks
      Jason Griffith

        Activity

        Hide
        Roshan Dawrani added a comment - - edited

        The issue is related to new optimization logic that determines whether to take a fast path or a slow path. I think Verifier#addFastPathHelperFieldsAndHelperMethod() and DGM#disablePrimitiveOptimization() are a little inconistently written.

        Say, we have class B inheriting from A.

        Verifier#addFastPathHelperFieldsAndHelperMethod() sees that A has the static field __$stMC (used in optimization related guards) because it also looks at super classes and then doesn't add it to B.

        On the other hand, DGM#disablePrimitiveOptimization() tries to set __$stMC on the current class only. But in above case, no __$stMC is added to class B, and attempt to disable the optimizations goes wasted (with NoSuchFieldException), and later on when that flag is looked at to see whether to dispatch the method call in "RemoteObject object = getRemoteObject()" in a fast way or slow way, it chooses the fast path because it finds the optimization still enabled by mistake.

        So, here is a patch that removes the above inconsistency between Verifier and DGM:

        @@ -17790,15 +17790,21 @@ public class DefaultGroovyMethods extends DefaultGroovyMethodsSupport {
             }
         
             private static void disablePrimitiveOptimization(Object self) {
                 Field sdyn;
                 Class c = self.getClass();
        -        try {
        -            sdyn = c.getDeclaredField(Verifier.STATIC_METACLASS_BOOL);
        -            sdyn.setBoolean(null, true);
        -        } catch (Throwable e) {
        -            //DO NOTHING
        +        while (c != null) {
        +            try {
        +                sdyn = c.getDeclaredField(Verifier.STATIC_METACLASS_BOOL);
        +                sdyn.setBoolean(null, true);
        +                break;
        +            } catch (NoSuchFieldException e) {
        +               c = c.getSuperclass();
        +               continue;
        +            } catch (Throwable e) {
        +                //DO NOTHING
        +            }
                 }
             }
         
             /**
              * Sets/updates the metaclass for a given class to a closure.
        

        Although I personally think it's the code in Verifier that is wrong and in adding the static field __$stMC, it should not look at super classes. Depends on bytecode optimization enabling/disabled is designed keeping what kind of behavior in mind when it comes to inheritence. If from A inherit B and C and we do some metaclass stuff to B (and hence disable the optimization in B), in current implementation it may disable the optimization for C also, I think, because the guard pick-up the information from A's __$stMC static field.

        Show
        Roshan Dawrani added a comment - - edited The issue is related to new optimization logic that determines whether to take a fast path or a slow path. I think Verifier#addFastPathHelperFieldsAndHelperMethod() and DGM#disablePrimitiveOptimization() are a little inconistently written. Say, we have class B inheriting from A. Verifier#addFastPathHelperFieldsAndHelperMethod() sees that A has the static field __$stMC (used in optimization related guards) because it also looks at super classes and then doesn't add it to B. On the other hand, DGM#disablePrimitiveOptimization() tries to set __$stMC on the current class only. But in above case, no __$stMC is added to class B, and attempt to disable the optimizations goes wasted (with NoSuchFieldException), and later on when that flag is looked at to see whether to dispatch the method call in "RemoteObject object = getRemoteObject()" in a fast way or slow way, it chooses the fast path because it finds the optimization still enabled by mistake. So, here is a patch that removes the above inconsistency between Verifier and DGM: @@ -17790,15 +17790,21 @@ public class DefaultGroovyMethods extends DefaultGroovyMethodsSupport { } private static void disablePrimitiveOptimization( Object self) { Field sdyn; Class c = self.getClass(); - try { - sdyn = c.getDeclaredField(Verifier.STATIC_METACLASS_BOOL); - sdyn.setBoolean( null , true ); - } catch (Throwable e) { - //DO NOTHING + while (c != null ) { + try { + sdyn = c.getDeclaredField(Verifier.STATIC_METACLASS_BOOL); + sdyn.setBoolean( null , true ); + break ; + } catch (NoSuchFieldException e) { + c = c.getSuperclass(); + continue ; + } catch (Throwable e) { + //DO NOTHING + } } } /** * Sets/updates the metaclass for a given class to a closure. Although I personally think it's the code in Verifier that is wrong and in adding the static field __$stMC, it should not look at super classes. Depends on bytecode optimization enabling/disabled is designed keeping what kind of behavior in mind when it comes to inheritence. If from A inherit B and C and we do some metaclass stuff to B (and hence disable the optimization in B), in current implementation it may disable the optimization for C also, I think, because the guard pick-up the information from A's __$stMC static field.
        Hide
        blackdrag blackdrag added a comment -

        Roshan you are completely right about the Verifier I would say. In B extends A I don't want to disable optimizations for A, just because B got changed.

        Show
        blackdrag blackdrag added a comment - Roshan you are completely right about the Verifier I would say. In B extends A I don't want to disable optimizations for A, just because B got changed.
        Hide
        Roshan Dawrani added a comment -

        Fixed by changing Verifier as discussed.

        Show
        Roshan Dawrani added a comment - Fixed by changing Verifier as discussed.
        Hide
        Jason Griffith added a comment -

        Tested the latest 1.8.3 snapshot with this bug - it's fixed on our end.

        It was very impressive how quickly you guys jumped on this and fixed it. Thanks!

        Show
        Jason Griffith added a comment - Tested the latest 1.8.3 snapshot with this bug - it's fixed on our end. It was very impressive how quickly you guys jumped on this and fixed it. Thanks!

          People

          • Assignee:
            Roshan Dawrani
            Reporter:
            Jason Griffith
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: