Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: fix ReflectionUtil throw unexpected exception #3803

Merged

Conversation

wangliang181230
Copy link
Contributor

@wangliang181230 wangliang181230 commented Jun 3, 2021

bugfix:

fix ReflectionUtil.invokeMethod will be invoke method multiple times, and fix the exception was lost when the method throws an exception

错误修复:

修复ReflectionUtil.invokeMethod方法的两个异常case:

  1. 业务方法抛出异常时,异常被吞,最终抛出的是NoSuchMethodException异常
    (预期:应该抛出包装了业务异常的InvocationTargetException异常)
  2. 业务方法被调用多次,满足以下两个条件会出现这个BUG:
    1)业务方法抛出异常
    2)业务类的superClass也有该方法。

其他优化:

  1. 重构了ReflectionUtil工具类的代码。

BUG的根本原因:

InvocationTargetException异常处理不当。


bug-1 复现代码:

// Service1.java
public class Service1 {

    public void test() {
        throw new RuntimeException("业务出错,抛出了异常");
    }

}

// Test1.java
public class Test1 {
    public static void main(String[] args) throws Exception {
        Service1 s = new Service1();
        ReflectionUtil.invokeMethod(s, "test"); // 查看控制台打印的异常信息是否为`InvocationTargetException`,如果不是,说明是有问题的。
    }
}

bug-2 复现代码:

// Service2.java
public class Service2 extends SuperService2 {
    private int i = 0;

    @Override
    public void plusOne() {
        i++;
        throw new RuntimeException("业务出错,抛出了异常");
    }

    public int getI() {
        return i;
    }
}

// SuperService2.java
public abstract class SuperService2 {
    abstract void plusOne();
}

// Test2.java
public class Test2 {
    public static void main(String[] args) throws Exception {
        Service s = new Service();
        try {
            ReflectionUtil.invokeMethod(s, "plusOne");
        } catch (Exception e) {
            e.printStackTrace();
        }
        System.out.println("i = " + s.getI()); // 查看输出结果是否为`i = 1`,如果不是,说明是有问题的。
    }
}

… once, when the underlying method throws an exception and the super class has the method too
@wangliang181230 wangliang181230 changed the title bugfix: fix ReflectionUtil.invokeMethod, the method will be called multiple times, if the method throws an exception and the superclass also has the method bugfix: fix ReflectionUtil.invokeMethod, the method will be called multiple times and the method exception was lost, if the method throws an exception and the superclass also has the method Jun 3, 2021
@wangliang181230 wangliang181230 added this to the 1.5.0 milestone Jun 3, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2021

Codecov Report

Merging #3803 (8991997) into develop (119f6eb) will decrease coverage by 0.06%.
The diff coverage is 46.52%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3803      +/-   ##
=============================================
- Coverage      40.72%   40.65%   -0.07%     
- Complexity      3005     3032      +27     
=============================================
  Files            676      677       +1     
  Lines          22660    22745      +85     
  Branches        2814     2831      +17     
=============================================
+ Hits            9228     9247      +19     
- Misses         12581    12633      +52     
- Partials         851      865      +14     
Impacted Files Coverage Δ
...src/main/java/io/seata/common/util/ArrayUtils.java 0.00% <0.00%> (ø)
.../src/main/java/io/seata/common/util/BeanUtils.java 67.12% <33.33%> (ø)
...main/java/io/seata/common/util/ReflectionUtil.java 59.71% <50.00%> (-34.89%) ⬇️
...rc/main/java/io/seata/common/util/StringUtils.java 28.63% <71.42%> (+2.05%) ⬆️

@wangliang181230 wangliang181230 changed the title bugfix: fix ReflectionUtil.invokeMethod, the method will be called multiple times and the method exception was lost, if the method throws an exception and the superclass also has the method bugfix: fix ReflectionUtil.invokeMethod, the method will be invoke multiple times and the method exception was lost, if the method throws an exception and the superclass also has the method Jun 4, 2021
// remove un_used fields
fieldList.removeIf(field -> field.getName().contains("$"));
// remove the static or synthetic fields
fieldList.removeIf(f -> Modifier.isStatic(f.getModifiers()) || f.isSynthetic());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review提示:
当前方法不应该读取静态字段,以及java自己生成的字段。

wangliang181230 added a commit to wangliang181230/seata that referenced this pull request Jun 4, 2021
…ix-ReflectionUtil.invokeMethod

# Conflicts:
#	common/src/main/java/io/seata/common/util/StringUtils.java
#	common/src/test/java/io/seata/common/util/StringUtilsTest.java
@wangliang181230 wangliang181230 changed the title bugfix: fix ReflectionUtil.invokeMethod, the method will be invoke multiple times and the method exception was lost, if the method throws an exception and the superclass also has the method bugfix: fix ReflectionUtil.invokeMethod will be invoke method multiple times, and fix the exception was lost when the method throws an exception Jun 17, 2021
Copy link
Contributor

@caohdgege caohdgege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@slievrly slievrly changed the title bugfix: fix ReflectionUtil.invokeMethod will be invoke method multiple times, and fix the exception was lost when the method throws an exception bugfix: fix ReflectionUtil throw unexpected exception Jul 13, 2021
@slievrly slievrly merged commit fe37fb3 into apache:develop Jul 13, 2021
@wangliang181230 wangliang181230 deleted the bugfix/fix-ReflectionUtil.invokeMethod branch July 13, 2021 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants