NPE Aspect - Githubissues

MrZhengliang commented 5 years ago

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 commented 5 years ago

@candyleer please recheck. Thanks.

candyleer commented 5 years ago

ok,I will recheck this today

candyleer commented 5 years ago

@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 commented 5 years ago

ok,demo Maybe at night.

MrZhengliang commented 5 years ago

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

candyleer commented 5 years ago

OK,Thanks a lot

candyleer commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

wu-sheng commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

OK, I know what you mean.

wu-sheng commented 5 years ago

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

apache / skywalking

NPE Aspect #3035

Question

Bug

Requirement or improvement