Skywalking: NPE Aspect

[MrZhengliang]

Please answer these questions before submitting your issue.

• Why do you submit this issue?
• [x] Question or discussion
• [ ] Bug
• [ ] Requirement
• [ ] Feature or performance improvement

---

Question

• What do you want to know?
  Isn't this bug solved?
  issue #1114
  Does this edition still have this problem

---

Bug

• Which version of SkyWalking, OS and JRE?
  apache-skywalking-apm-6.1.0
  Centos，MacOS
  JRE1.8

• Which company or project?

• What happen?
  If possible, provide a way for reproducing the error. e.g. demo application, component version.
  `@Pointcut("execution(public * com.lea.scm.web...(..))")
  public void log() {
  }

  @Before("log()")
  public void exBefore(JoinPoint joinPoint) {
  ServletRequestAttributes sra = (ServletRequestAttributes) RequestContextHolder.getRequestAttributes();
  HttpServletRequest request = sra.getRequest();
  logger.info("url:" + request.getRequestURL());
  logger.info("method:" + request.getMethod());
  logger.info("class method:" + joinPoint.getSignature().getDeclaringTypeName() + "." + joinPoint.getSignature().getName());
  if(!ObjectUtils.isEmpty(joinPoint.getArgs())){
  logger.info("args:" + JsonUtils.toJson(joinPoint.getArgs()));
  }
  }`

java.lang.NullPointerException: null at com.lea.scm.web.GoodsExternalController.shopGoodsListPage$original$TiDS4VBn(GoodsExternalController.java:110) at com.lea.scm.web.GoodsExternalController.shopGoodsListPage$original$TiDS4VBn$accessor$M9qsnZ2l(GoodsExternalController.java) at com.lea.scm.web.GoodsExternalController$auxiliary$VfxWxE5S.call(Unknown Source) at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstMethodsInter.intercept(InstMethodsInter.java:93) at com.lea.scm.web.GoodsExternalController.shopGoodsListPage(GoodsExternalController.java) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(InvocableHandlerMethod.java:205) at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest$original$YWb59KLP(InvocableHandlerMethod.java:133) at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest$original$YWb59KLP$accessor$43vOj5WC(InvocableHandlerMethod.java) at org.springframework.web.method.support.InvocableHandlerMethod$auxiliary$dFEFp2LA.call(Unknown Source) at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstMethodsInter.intercept(InstMethodsInter.java:93) at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java) at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:97) at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:827) at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:738) at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:85) at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:967) at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:901) at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:970) at org.springframework.web.servlet.FrameworkServlet.doPost(FrameworkServlet.java:872) at javax.servlet.http.HttpServlet.service(HttpServlet.java:661) at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:846) at javax.servlet.http.HttpServlet.service(HttpServlet.java:742) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:231) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.boot.web.filter.ApplicationContextHeaderFilter.doFilterInternal(ApplicationContextHeaderFilter.java:55) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.boot.actuate.trace.WebRequestTraceFilter.doFilterInternal(WebRequestTraceFilter.java:111) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:99) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.web.filter.HttpPutFormContentFilter.doFilterInternal(HttpPutFormContentFilter.java:105) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.web.filter.HiddenHttpMethodFilter.doFilterInternal(HiddenHttpMethodFilter.java:81) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.cloud.sleuth.instrument.web.TraceFilter.doFilter(TraceFilter.java:166) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:197) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.boot.actuate.autoconfigure.MetricsFilter.doFilterInternal(MetricsFilter.java:103) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:198) at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96) at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:493) at org.apache.catalina.core.StandardHostValve.invoke$original$NsgxOShB(StandardHostValve.java:140) at org.apache.catalina.core.StandardHostValve.invoke$original$NsgxOShB$accessor$zQGbHoR0(StandardHostValve.java) at org.apache.catalina.core.StandardHostValve$auxiliary$CrZwynl4.call(Unknown Source) at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstMethodsInter.intercept(InstMethodsInter.java:93) at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java) at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:81) at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:87) at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:342) at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:800) at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:66) at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:806) at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1498) at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) at java.lang.Thread.run(Thread.java:748)

---

Requirement or improvement

• Please describe about your requirements or improvement suggestions.

[wu-sheng]

@candyleer please recheck. Thanks.

[candyleer]

ok,I will recheck this today

[candyleer]

@MrZhengliang I think it's the problem of your business code,cause from your log ,the line number(93) is call your own method that throw NPE.
the log say the NPE exist in GoodsExternalController.java:110,method is shopGoodsListPage
If not, can you provide a demo contains the controller together?

[MrZhengliang]

ok,demo Maybe at night.

[MrZhengliang]

demo is https://github.com/MrZhengliang/demo-skywalking-service

[candyleer]

OK,Thanks a lot

[candyleer]

Actually, this is a complex problem.
1.the controller method modifier is private which is not recommend for controller, without skywalking agent it is working just because an accident, you can try add a public method in the controller ,you will see the goodsExternalService is always null in private method.
1.1 why the goodsExternalService is always null in private method if add another public method?
the reason is the controller is enhanced by cglib in spring ,in enhanced classes ,private method cannot get goodsExternalService
1.2 why it can work in current version?
the reason is the controller just have a method and is private and it is not match the ascpect expression,so the controller will not be enhanced by cglib in spring .

2.why with skywalking agent it doesn't work?
the reason is is have implement an interface EnhanceInstance and it have two default method getSkywalkingField and setSkywalkingField which modifier is public ,this match the aspect expression.so the controller will enhanced by cglib in spring .

3.conclusion
If the class is enhanced by cglib in spring .all private method will get goodsExternalService by null.

[candyleer]

So for your case,I recommend you to update the controller method to public modifier will resolve this problem.
actually ,public modifier for controller ReqeustMapping method should be best practice.

If the application work with agent should work as same as work without agent, I think we should not implement EnhanceInstance interface. but this should be long discussed.

[arugal]

spring-boot-starter-aop:2.1.1.RELEAS

@SpringBootApplication
public class AopApplication implements CommandLineRunner {

    public static void main(String[] args) {
        SpringApplication.run(AopApplication.class);
    }

    @Autowired
    private Service service;

    @Override
    public void run(String... args) throws Exception {
        System.out.println("autowired:" + System.identityHashCode(service));

        Method method1 = Service.class.getDeclaredMethod("method1");
        method1.setAccessible(true);
        method1.invoke(service);

        Method method2 = Service.class.getDeclaredMethod("method2");
        method2.setAccessible(true);
        method2.invoke(service);
    }
}
@Aspect
@Component
public class AopAspect {

    @Pointcut("execution(* com.github.sunnus3.example.spring.Service.*(..))")
    public void aop() {

    }

    @Before("aop()")
    public void before(JoinPoint joinPoint) {
        System.out.println("before ...");
    }
}
@Component
public class Service {


    private void method1() {
        System.out.println("private method: " + System.identityHashCode(this));
    }

    public void method2() {
        System.out.println("public  method: " + System.identityHashCode(this));
    }
}

Result
autowired:680988889
private method: 680988889
before ...
public  method: 1792227359

[MrZhengliang]

Thanks a lot! Our revision was successful . We'll be more precise when coding this piece！

[wu-sheng]

@candyleer I noticed this discussion. It looks like a little complex, if we supports don't implement EnhancedInstance, which is easy, but how do we do Spring plugin in good way?

Do you think we worth to open an issue to discuss this and add a FAQ document for next user?

[candyleer]

@wu-sheng I think the best way is do not implement EnhancedInstance(cause spring has some dependcy for interface,if we want make the program work as same as without skywalking agent), we can cache the method and path in the static map cache,and provide do not implement EnhancedInstance mechanism can solve some other problem, such as dubbo-conflict-plugin(which I think it has some side effect if dubbo change the patch class we do)
so I am prepare working on do not implement EnhancedInstance way for springmvc first

[wu-sheng]

@candyleer In the old day(not in SkyWalking), I check about how to do Spring Controller plugin, the reason of EnhancedInstance is that there is no place to put mapping path cache, so most likely, you have to maintain a static map to do so, which cause the performance concern in big system(10k+ Controller methods at least)

I am totally aware this kind of OP has risk, but, actually, if we consider this risk, we can't do anything at any class because any class could be added in Spring context by the end user, we don't know.

This is why we added Spring patch at first place, could you try to enhance it rather than provide a plugin or remove this field? I am going to open a new issue to discuss this.

[candyleer]

OK, I know what you mean.

[wu-sheng]

@MrZhengliang We provided a new patch, if you have time, could you test with your old codes by using master branch agent?