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

NPE in TimedAspect (JdkDynamicAopProxy) #3190

Closed
aleksey-astashenko-jemmic opened this issue May 19, 2022 · 3 comments · Fixed by #3195
Closed

NPE in TimedAspect (JdkDynamicAopProxy) #3190

aleksey-astashenko-jemmic opened this issue May 19, 2022 · 3 comments · Fixed by #3195
Labels
bug A general bug module: micrometer-core An issue that is related to our core module
Milestone

Comments

@aleksey-astashenko-jemmic

Describe the bug
We had some implementation classes annotated with @timed annotation. After migrating to 1.9.0 from 1.8.5 we're having NPE in TimedAspect class. It happens because of the part of the code from io.micrometer.core.aop.TimedAspect#timedClass method:

        Method method = ((MethodSignature) pjp.getSignature()).getMethod();
        Class<?> declaringClass = method.getDeclaringClass();
        Timed timed = declaringClass.getAnnotation(Timed.class);

Method being called through the interface reference, and the @timed annotation is actually on the implementation class itself. Therefore, declaringClass resolved into interface class, and timed variable is null. Then we get NPE down the stack.

Am I correct in assuming that we should have here some additional logic like in the io.micrometer.core.aop.TimedAspect#timedMethod metod? This is how it looks like:

        if (timed == null) {
            method = pjp.getTarget().getClass().getMethod(method.getName(), method.getParameterTypes());
            timed = method.getAnnotation(Timed.class);
        }

Additionally, seems like we're missing shouldSkip predicate check in io.micrometer.core.aop.TimedAspect#timedClass, although we have it in the io.micrometer.core.aop.TimedAspect#timedMethod:

        if (shouldSkip.test(pjp)) {
            return pjp.proceed();
        }

Environment
Micrometer version [1.9.0] (works on [1.8.5])
Micrometer registry [Prometheus]
OS: [Windows/Linux]

Additional context
It appears that it's related to this recently merged PR
Also little bit related to this issue

@shakuzen shakuzen added bug A general bug module: micrometer-core An issue that is related to our core module labels May 20, 2022
@shakuzen shakuzen added this to the 1.9.x milestone May 20, 2022
@shakuzen
Copy link
Member

Thank you for the detailed report and sorry for the issues. It sounds like you've got the right idea with the fixes. Would you be interested in contributing pull requests? Ideally two separate pull requests for the two separate issues.

@aleksey-astashenko-jemmic
Copy link
Author

@shakuzen , thank you for the quick response, and sorry for the delay from my side. Created two pull requests for both issues presented: #3194, #3195

@shakuzen shakuzen modified the milestones: 1.9.x, 1.9.1 May 26, 2022
@shakuzen
Copy link
Member

Thank you for reporting the issues and contributing the fixes. They are now available in snapshots and will be in the upcoming 1.9.1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants