groovy

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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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.
Roshan Dawrani made changes -
Field Original Value New Value
Assignee Roshan Dawrani [ roshandawrani ]
Hide
Permalink
Roshan Dawrani added a comment -

Fixed by changing Verifier as discussed.

Show
Roshan Dawrani added a comment - Fixed by changing Verifier as discussed.
Roshan Dawrani made changes -
Status Open [ 1 ] Resolved [ 5 ]
Fix Version/s 1.8.3 [ 17657 ]
Fix Version/s 1.9-beta-4 [ 17656 ]
Resolution Fixed [ 1 ]
Hide
Permalink
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!
Paul King made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

  • Assignee:
    Roshan Dawrani
    Reporter:
    Jason Griffith
Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: