Fix NullPointerException on tomcat (#3705)

* Fix NullPointerException on tomcat * remove commented out line * instrument CoyoteAdapter
open-telemetry · Jul 28, 2021 · d73e030 · d73e030
1 parent d305f31
commit d73e030
Show file tree

Hide file tree

Showing 13 changed files with 492 additions and 30 deletions.
diff --git a/...library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java b/...library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java
@@ -31,8 +31,6 @@ public void addRequestAsyncListener(
       request
           .getAsyncContext()
           .addListener(new Listener(listener), request, (HttpServletResponse) response);
-    } else {
-      request.getAsyncContext().addListener(new Listener(listener));
     }
   }
 

diff --git a/...in/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java b/...in/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java
@@ -109,8 +109,6 @@ public void addRequestAsyncListener(
       request
           .getAsyncContext()
           .addListener(new Listener(listener), request, (HttpServletResponse) response);
-    } else {
-      request.getAsyncContext().addListener(new Listener(listener));
     }
   }
 

diff --git a/...io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10AttachResponseAdvice.java b/...io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10AttachResponseAdvice.java
@@ -0,0 +1,30 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0;
+
+import io.opentelemetry.instrumentation.servlet.jakarta.v5_0.JakartaServletHttpServerTracer;
+import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServerHandlerAdviceHelper;
+import net.bytebuddy.asm.Advice;
+import org.apache.coyote.Request;
+import org.apache.coyote.Response;
+
+public class Tomcat10AttachResponseAdvice {
+
+  @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
+  public static void attachResponse(
+      @Advice.Argument(0) Request request,
+      @Advice.Argument(2) Response response,
+      @Advice.Return boolean success) {
+
+    if (success) {
+      TomcatServerHandlerAdviceHelper.attachResponseToRequest(
+          Tomcat10ServletEntityProvider.INSTANCE,
+          JakartaServletHttpServerTracer.tracer(),
+          request,
+          response);
+    }
+  }
+}
diff --git a/...o/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10InstrumentationModule.java b/...o/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10InstrumentationModule.java
@@ -6,12 +6,12 @@
 package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0;
 
 import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
+import static java.util.Collections.singletonList;
 
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServerHandlerInstrumentation;
-import java.util.Collections;
 import java.util.List;
 import net.bytebuddy.matcher.ElementMatcher;
 
@@ -30,9 +30,10 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 
   @Override
   public List<TypeInstrumentation> typeInstrumentations() {
-    return Collections.singletonList(
+    String packageName = Tomcat10InstrumentationModule.class.getPackage().getName();
+    return singletonList(
         new TomcatServerHandlerInstrumentation(
-            Tomcat10InstrumentationModule.class.getPackage().getName()
-                + ".Tomcat10ServerHandlerAdvice"));
+            packageName + ".Tomcat10ServerHandlerAdvice",
+            packageName + ".Tomcat10AttachResponseAdvice"));
   }
 }
diff --git a/.../io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java b/.../io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java
@@ -31,12 +31,6 @@ public static void onEnter(
     context = tracer().startServerSpan(request);
 
     scope = context.makeCurrent();
-
-    TomcatServerHandlerAdviceHelper.attachResponseToRequest(
-        Tomcat10ServletEntityProvider.INSTANCE,
-        JakartaServletHttpServerTracer.tracer(),
-        request,
-        response);
   }
 
   @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)

diff --git a/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy b/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy
@@ -0,0 +1,73 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0
+
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS
+
+import io.opentelemetry.instrumentation.test.base.HttpServerTest
+import jakarta.servlet.annotation.WebServlet
+import jakarta.servlet.http.HttpServlet
+import jakarta.servlet.http.HttpServletRequest
+import jakarta.servlet.http.HttpServletResponse
+import java.util.concurrent.Phaser
+
+@WebServlet(asyncSupported = true)
+class AsyncServlet extends HttpServlet {
+  @Override
+  protected void service(HttpServletRequest req, HttpServletResponse resp) {
+    HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath)
+    def phaser = new Phaser(2)
+    def context = req.startAsync()
+    context.start {
+      try {
+        phaser.arriveAndAwaitAdvance()
+        HttpServerTest.controller(endpoint) {
+          resp.contentType = "text/plain"
+          switch (endpoint) {
+            case SUCCESS:
+              resp.status = endpoint.status
+              resp.writer.print(endpoint.body)
+              context.complete()
+              break
+            case INDEXED_CHILD:
+              endpoint.collectSpanAttributes { req.getParameter(it) }
+              resp.status = endpoint.status
+              context.complete()
+              break
+            case QUERY_PARAM:
+              resp.status = endpoint.status
+              resp.writer.print(req.queryString)
+              context.complete()
+              break
+            case REDIRECT:
+              resp.sendRedirect(endpoint.body)
+              context.complete()
+              break
+            case ERROR:
+              resp.status = endpoint.status
+              resp.writer.print(endpoint.body)
+              context.complete()
+              break
+            case EXCEPTION:
+              resp.status = endpoint.status
+              resp.writer.print(endpoint.body)
+              context.complete()
+              throw new Exception(endpoint.body)
+          }
+        }
+      } finally {
+        phaser.arriveAndDeregister()
+      }
+    }
+    phaser.arriveAndAwaitAdvance()
+    phaser.arriveAndAwaitAdvance()
+  }
+}
diff --git a/...est/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.groovy b/...est/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.groovy
@@ -0,0 +1,129 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0
+
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS
+
+import io.opentelemetry.instrumentation.test.AgentTestTrait
+import io.opentelemetry.instrumentation.test.asserts.TraceAssert
+import io.opentelemetry.instrumentation.test.base.HttpServerTest
+import jakarta.servlet.Servlet
+import jakarta.servlet.ServletException
+import java.nio.file.Files
+import org.apache.catalina.Context
+import org.apache.catalina.startup.Tomcat
+import org.apache.tomcat.JarScanFilter
+import org.apache.tomcat.JarScanType
+import spock.lang.Unroll
+
+@Unroll
+class TomcatAsyncTest extends HttpServerTest<Tomcat> implements AgentTestTrait {
+
+  @Override
+  Tomcat startServer(int port) {
+    def tomcatServer = new Tomcat()
+
+    def baseDir = Files.createTempDirectory("tomcat").toFile()
+    baseDir.deleteOnExit()
+    tomcatServer.setBaseDir(baseDir.getAbsolutePath())
+
+    tomcatServer.setPort(port)
+    tomcatServer.getConnector().enableLookups = true // get localhost instead of 127.0.0.1
+
+    File applicationDir = new File(baseDir, "/webapps/ROOT")
+    if (!applicationDir.exists()) {
+      applicationDir.mkdirs()
+      applicationDir.deleteOnExit()
+    }
+    Context servletContext = tomcatServer.addWebapp(contextPath, applicationDir.getAbsolutePath())
+    // Speed up startup by disabling jar scanning:
+    servletContext.getJarScanner().setJarScanFilter(new JarScanFilter() {
+      @Override
+      boolean check(JarScanType jarScanType, String jarName) {
+        return false
+      }
+    })
+
+    setupServlets(servletContext)
+
+    tomcatServer.start()
+
+    return tomcatServer
+  }
+
+  @Override
+  void stopServer(Tomcat server) {
+    server.stop()
+    server.destroy()
+  }
+
+  @Override
+  String getContextPath() {
+    return "/tomcat-context"
+  }
+
+  protected void setupServlets(Context context) {
+    def servlet = servlet()
+
+    addServlet(context, SUCCESS.path, servlet)
+    addServlet(context, QUERY_PARAM.path, servlet)
+    addServlet(context, ERROR.path, servlet)
+    addServlet(context, EXCEPTION.path, servlet)
+    addServlet(context, REDIRECT.path, servlet)
+    addServlet(context, AUTH_REQUIRED.path, servlet)
+    addServlet(context, INDEXED_CHILD.path, servlet)
+  }
+
+  void addServlet(Context servletContext, String path, Class<Servlet> servlet) {
+    String name = UUID.randomUUID()
+    Tomcat.addServlet(servletContext, name, servlet.newInstance())
+    servletContext.addServletMappingDecoded(path, name)
+  }
+
+  Class<Servlet> servlet() {
+    AsyncServlet
+  }
+
+  @Override
+  boolean testException() {
+    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
+    return false
+  }
+
+  @Override
+  boolean testConcurrency() {
+    return true
+  }
+
+  @Override
+  Class<?> expectedExceptionClass() {
+    ServletException
+  }
+
+  @Override
+  boolean hasResponseSpan(ServerEndpoint endpoint) {
+    endpoint == NOT_FOUND || endpoint == REDIRECT
+  }
+
+  @Override
+  void responseSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) {
+    switch (endpoint) {
+      case REDIRECT:
+        redirectSpan(trace, index, parent)
+        break
+      case NOT_FOUND:
+        sendErrorSpan(trace, index, parent)
+        break
+    }
+  }
+}
diff --git a/...a/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7AttachResponseAdvice.java b/...a/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7AttachResponseAdvice.java
@@ -0,0 +1,30 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.tomcat.v7_0;
+
+import io.opentelemetry.instrumentation.servlet.v3_0.Servlet3HttpServerTracer;
+import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServerHandlerAdviceHelper;
+import net.bytebuddy.asm.Advice;
+import org.apache.coyote.Request;
+import org.apache.coyote.Response;
+
+public class Tomcat7AttachResponseAdvice {
+
+  @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
+  public static void attachResponse(
+      @Advice.Argument(0) Request request,
+      @Advice.Argument(2) Response response,
+      @Advice.Return boolean success) {
+
+    if (success) {
+      TomcatServerHandlerAdviceHelper.attachResponseToRequest(
+          Tomcat7ServletEntityProvider.INSTANCE,
+          Servlet3HttpServerTracer.tracer(),
+          request,
+          response);
+    }
+  }
+}
diff --git a/.../io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7InstrumentationModule.java b/.../io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7InstrumentationModule.java
@@ -6,13 +6,13 @@
 package io.opentelemetry.javaagent.instrumentation.tomcat.v7_0;
 
 import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
+import static java.util.Collections.singletonList;
 import static net.bytebuddy.matcher.ElementMatchers.not;
 
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServerHandlerInstrumentation;
-import java.util.Collections;
 import java.util.List;
 import net.bytebuddy.matcher.ElementMatcher;
 
@@ -33,9 +33,10 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
   public List<TypeInstrumentation> typeInstrumentations() {
     // Tomcat 10+ is excluded by making sure Request does not have any methods returning
     // jakarta.servlet.ReadListener which is returned by getReadListener method on Tomcat 10+
-    return Collections.singletonList(
+    String packageName = Tomcat7InstrumentationModule.class.getPackage().getName();
+    return singletonList(
         new TomcatServerHandlerInstrumentation(
-            Tomcat7InstrumentationModule.class.getPackage().getName()
-                + ".Tomcat7ServerHandlerAdvice"));
+            packageName + ".Tomcat7ServerHandlerAdvice",
+            packageName + ".Tomcat7AttachResponseAdvice"));
   }
 }
diff --git a/...va/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServerHandlerAdvice.java b/...va/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServerHandlerAdvice.java
@@ -31,12 +31,6 @@ public static void onEnter(
     context = tracer().startServerSpan(request);
 
     scope = context.makeCurrent();
-
-    TomcatServerHandlerAdviceHelper.attachResponseToRequest(
-        Tomcat7ServletEntityProvider.INSTANCE,
-        Servlet3HttpServerTracer.tracer(),
-        request,
-        response);
   }
 
   @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)