groovy
  1. groovy
  2. GROOVY-5202

inherited non public listener structure causing NPE

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.4
    • Fix Version/s: 1.8.6, 2.0-beta-3, 1.7.11
    • Component/s: None
    • Labels:
      None
    • Environment:
      Windows XP, Oracle 11.2.0.2
    • Number of attachments :
      2

      Description

      the following script works with groovy 1.8.2 but not with 1.8.4 oci connection has an error (oci_error.txt) – thin connection works

      import groovy.sql.Sql
      import java.sql.SQLException
      def dbc = Sql.newInstance('jdbc:oracle:oci:@b003', 'system', 'system', "oracle.jdbc.driver.OracleDriver")
      //def dbc = Sql.newInstance('jdbc:oracle:thin:@//server:1521/b003', 'system', 'system', "oracle.jdbc.driver.OracleDriver")
      println dbc.properties
      def row = dbc.firstRow("SELECT count(*) FROM all_users WHERE username = 'SYSTEM'")
      println row
      dbc.connection.autoCommit = false
      

        Activity

        Hide
        John Wagenleitner added a comment - - edited

        The problem appears to be with processing EventSetDescriptors. The OCI driver uses the T2CConnection whose super class PhysicalConnection contains add/remove event listener methods. The base class is package private and abstract. The thin driver uses T4CConnection and that class overrides the addXSEventListener methods, so that is likely why the thin driver does not exhibit the problem.

        I was able to create some sample classes that reproduce the problem (Groovy 1.8.3+ and JDK 6 and 7):

        1. unzip GroovyBugEventListeners.zip
        2. cd GroovyBugEventListeners/
        3. javac example/*
        4. groovy AbstractBaseWithEventListeners.groovy

        Note that if you either make FooBaseConnection public or change the event listeners from the FooEventListener interface to the java.util.EventListener you no longer get the NPE.

        Show
        John Wagenleitner added a comment - - edited The problem appears to be with processing EventSetDescriptors. The OCI driver uses the T2CConnection whose super class PhysicalConnection contains add/remove event listener methods. The base class is package private and abstract. The thin driver uses T4CConnection and that class overrides the addXSEventListener methods, so that is likely why the thin driver does not exhibit the problem. I was able to create some sample classes that reproduce the problem (Groovy 1.8.3+ and JDK 6 and 7): unzip GroovyBugEventListeners.zip cd GroovyBugEventListeners/ javac example/* groovy AbstractBaseWithEventListeners.groovy Note that if you either make FooBaseConnection public or change the event listeners from the FooEventListener interface to the java.util.EventListener you no longer get the NPE.
        Hide
        blackdrag blackdrag added a comment -

        the error seems to appear in code from this:

                EventSetDescriptor[] eventDescriptors = info.getEventSetDescriptors();
                for (EventSetDescriptor descriptor : eventDescriptors) {
                    Method[] listenerMethods = descriptor.getListenerMethods();
                    for (Method listenerMethod : listenerMethods) {
                        final MetaMethod metaMethod = CachedMethod.find(descriptor.getAddListenerMethod());
                        addToAllMethodsIfPublic(metaMethod);
                        String name = listenerMethod.getName();
                        if (listeners.containsKey(name)) {
                            listeners.put(name, AMBIGUOUS_LISTENER_METHOD);
                        } else {
                            listeners.put(name, metaMethod);
                        }
                    }
                }

        because in addToAllMethodsIfPublic we do

            private void addToAllMethodsIfPublic(MetaMethod metaMethod) {
                if (Modifier.isPublic(metaMethod.getModifiers()))
                    allMethods.add(metaMethod);
            }

        And I think it is the metaMethod.getModifiers(), that is causing the NPE. A quick fix certainly would be to say we just skip the iteration step if we don't find the method and then not enter addToAllMethodsIfPublic at all. I guess for a non public listener it might be possible that you don't find such a method, especially in a subclass.

        Show
        blackdrag blackdrag added a comment - the error seems to appear in code from this: EventSetDescriptor[] eventDescriptors = info.getEventSetDescriptors(); for (EventSetDescriptor descriptor : eventDescriptors) { Method[] listenerMethods = descriptor.getListenerMethods(); for (Method listenerMethod : listenerMethods) { final MetaMethod metaMethod = CachedMethod.find(descriptor.getAddListenerMethod()); addToAllMethodsIfPublic(metaMethod); String name = listenerMethod.getName(); if (listeners.containsKey(name)) { listeners.put(name, AMBIGUOUS_LISTENER_METHOD); } else { listeners.put(name, metaMethod); } } } because in addToAllMethodsIfPublic we do private void addToAllMethodsIfPublic(MetaMethod metaMethod) { if (Modifier.isPublic(metaMethod.getModifiers())) allMethods.add(metaMethod); } And I think it is the metaMethod.getModifiers(), that is causing the NPE. A quick fix certainly would be to say we just skip the iteration step if we don't find the method and then not enter addToAllMethodsIfPublic at all. I guess for a non public listener it might be possible that you don't find such a method, especially in a subclass.
        Hide
        John Wagenleitner added a comment -

        You are right, metaMethod is null so it is the getModifiers() generating the NPE. The descriptor.getAddListenerMethod() returns public void groovy.bugs.example.FooConnection.addFooEventListener(groovy.bugs.example.FooEventListener), but the CacheMethod#find(Method) lookup only includes methods from the declaring class in this case. By the time it reaches here, the method public void groovy.bugs.example.FooBaseConnection.addFooEventListener(groovy.bugs.example.FooEventListener) from the superclass has already been added to the all methods. So with a null check in place it just does not add the method name to the listeners map.

        Show
        John Wagenleitner added a comment - You are right, metaMethod is null so it is the getModifiers() generating the NPE. The descriptor.getAddListenerMethod() returns public void groovy.bugs.example.FooConnection.addFooEventListener(groovy.bugs.example.FooEventListener) , but the CacheMethod#find(Method) lookup only includes methods from the declaring class in this case. By the time it reaches here, the method public void groovy.bugs.example.FooBaseConnection.addFooEventListener(groovy.bugs.example.FooEventListener) from the superclass has already been added to the all methods. So with a null check in place it just does not add the method name to the listeners map.
        Hide
        John Wagenleitner added a comment -

        I checked out the GROOVY_1_8_2 tag and found that it works because methods in the code below includes the public methods from the abstract super class.

        groovy.lang.MetaClassImpl
        public static CachedMethod find(Method method) {
           CachedMethod[] methods = ReflectionCache.getCachedClass(method.getDeclaringClass()).getMethods();
        

        So 1.8.2 methods is

        1. public void groovy.bugs.example.FooConnection.addFooEventListener(groovy.bugs.example.FooEventListener)
        2. public void groovy.bugs.example.FooConnection.removeFooEventListener(groovy.bugs.example.FooEventListener)
        3. public java.lang.String groovy.bugs.example.FooConnection.someMethod()

        And in master it is

        1. public java.lang.String groovy.bugs.example.FooConnection.someMethod()
        Show
        John Wagenleitner added a comment - I checked out the GROOVY_1_8_2 tag and found that it works because methods in the code below includes the public methods from the abstract super class. groovy.lang.MetaClassImpl public static CachedMethod find(Method method) { CachedMethod[] methods = ReflectionCache.getCachedClass(method.getDeclaringClass()).getMethods(); So 1.8.2 methods is public void groovy.bugs.example.FooConnection.addFooEventListener(groovy.bugs.example.FooEventListener) public void groovy.bugs.example.FooConnection.removeFooEventListener(groovy.bugs.example.FooEventListener) public java.lang.String groovy.bugs.example.FooConnection.someMethod() And in master it is public java.lang.String groovy.bugs.example.FooConnection.someMethod()
        Hide
        blackdrag blackdrag added a comment -

        I updated the title to reflect the issue at hand better, since it is not really SQL specific

        Show
        blackdrag blackdrag added a comment - I updated the title to reflect the issue at hand better, since it is not really SQL specific
        Hide
        blackdrag blackdrag added a comment -

        should be fixed now

        Show
        blackdrag blackdrag added a comment - should be fixed now
        Hide
        John Wagenleitner added a comment -

        I admit I don't know much about event listener handling and this is probably such an edge case it does not matter much, the null check is probably just fine. However, I was just curious about the problem so ran a git bisect to track the commit that caused this to no longer work (broke sometime after 1.8.2).

        Commit 3c47089c9573df7d01227f07d3a6fac5886a61eb introduced this problem by no longer adding these types of methods to the cached class. To get a good view of the change you can run:

        git show -w 3c47089c9573df7d01227f07d3a6fac5886a61eb
        commit 3c47089c9573df7d01227f07d3a6fac5886a61eb
        Author: Guillaume Laforge <glaforge@gmail.com>
        Date:   Thu Sep 22 21:51:48 2011 +0200
        
            GROOVY-5003: Fix the problem in handling bridge methods
        
        diff --git a/src/main/org/codehaus/groovy/reflection/CachedClass.java b/src/main/org/codehaus/groovy/reflection/CachedCl
        index 139babb..ec6d4da 100644
        --- a/src/main/org/codehaus/groovy/reflection/CachedClass.java
        +++ b/src/main/org/codehaus/groovy/reflection/CachedClass.java
        @@ -96,7 +96,7 @@ public class CachedClass {
                         final CachedMethod cachedMethod = new CachedMethod(CachedClass.this, declaredMethods[i]);
                         final String name = cachedMethod.getName();
         
        -                if (name.indexOf('+') >= 0) {
        +                if (declaredMethods[i].isBridge() || name.indexOf('+') >= 0) {
                             // Skip Synthetic methods inserted by JDK 1.5 compilers and later
                             continue;
                         } /*else if (Modifier.isAbstract(reflectionMethod.getModifiers())) {
        

        The current fix that is probably fine, but I found it interesting since the inherited methods are public as part of the subclass and it seems as if they should be part of the cached class. But again, this is territory I'm not very familiar with so just wanted make note of what caused it break between versions 1.8.2 and 1.8.3.

        Show
        John Wagenleitner added a comment - I admit I don't know much about event listener handling and this is probably such an edge case it does not matter much, the null check is probably just fine. However, I was just curious about the problem so ran a git bisect to track the commit that caused this to no longer work (broke sometime after 1.8.2). Commit 3c47089c9573df7d01227f07d3a6fac5886a61eb introduced this problem by no longer adding these types of methods to the cached class. To get a good view of the change you can run: git show -w 3c47089c9573df7d01227f07d3a6fac5886a61eb commit 3c47089c9573df7d01227f07d3a6fac5886a61eb Author: Guillaume Laforge <glaforge@gmail.com> Date: Thu Sep 22 21:51:48 2011 +0200 GROOVY-5003: Fix the problem in handling bridge methods diff --git a/src/main/org/codehaus/groovy/reflection/CachedClass.java b/src/main/org/codehaus/groovy/reflection/CachedCl index 139babb..ec6d4da 100644 --- a/src/main/org/codehaus/groovy/reflection/CachedClass.java +++ b/src/main/org/codehaus/groovy/reflection/CachedClass.java @@ -96,7 +96,7 @@ public class CachedClass { final CachedMethod cachedMethod = new CachedMethod(CachedClass. this , declaredMethods[i]); final String name = cachedMethod.getName(); - if (name.indexOf('+') >= 0) { + if (declaredMethods[i].isBridge() || name.indexOf('+') >= 0) { // Skip Synthetic methods inserted by JDK 1.5 compilers and later continue ; } /* else if (Modifier.isAbstract(reflectionMethod.getModifiers())) { The current fix that is probably fine, but I found it interesting since the inherited methods are public as part of the subclass and it seems as if they should be part of the cached class. But again, this is territory I'm not very familiar with so just wanted make note of what caused it break between versions 1.8.2 and 1.8.3.
        Hide
        John Wagenleitner added a comment -

        Just read up on bridge methods and guess it does make sense to skip them.

        Thanks for the fix.

        Show
        John Wagenleitner added a comment - Just read up on bridge methods and guess it does make sense to skip them. Thanks for the fix.
        Hide
        Ludger Wolter added a comment -

        I have test it with version 1.8.6 successfully. Thank you very much.

        Show
        Ludger Wolter added a comment - I have test it with version 1.8.6 successfully. Thank you very much.

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Ludger Wolter
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: