diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/build.gradle b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/build.gradle index 29dbc60e491..13fd18489bc 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/build.gradle +++ b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/build.gradle @@ -113,6 +113,7 @@ dependencies { testRuntimeOnly project(':dd-java-agent:instrumentation:websocket:jakarta-websocket-2.0') testImplementation testFixtures(project(':dd-java-agent:instrumentation:servlet:jakarta-servlet-5.0')) + testImplementation libs.bundles.junit5 tomcat9TestImplementation group: 'org.apache.tomcat.embed', name: 'tomcat-embed-core', version: '9.+' tomcat9TestImplementation group: 'org.apache.tomcat.embed', name: 'tomcat-embed-websocket', version: '9.+' diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java index 14fb92e8fae..539f2ba8612 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java @@ -133,7 +133,12 @@ public static void extractParent( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void closeScope(@Advice.Local("parentScope") ContextScope scope) { - scope.close(); + // scope can be null if extractParent() above threw before assigning it (the throwable is + // swallowed by suppress = Throwable.class), which would otherwise NPE here and mask the + // real failure. + if (scope != null) { + scope.close(); + } } } @@ -167,7 +172,12 @@ public static void onService( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void closeScope(@Advice.Local("serverScope") ContextScope serverScope) { - serverScope.close(); + // serverScope can be null if onService() above threw before assigning it (the throwable is + // swallowed by suppress = Throwable.class), which would otherwise NPE here and mask the + // real failure. + if (serverScope != null) { + serverScope.close(); + } } private void muzzleCheck(CoyoteAdapter adapter, Request request, Response response) diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/test/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentationTest.java b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/test/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentationTest.java new file mode 100644 index 00000000000..8046619c589 --- /dev/null +++ b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/test/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentationTest.java @@ -0,0 +1,49 @@ +package datadog.trace.instrumentation.tomcat; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.context.Context; +import datadog.context.ContextScope; +import org.junit.jupiter.api.Test; + +class TomcatServerInstrumentationTest { + + private static final class RecordingScope implements ContextScope { + private boolean closed; + + @Override + public Context context() { + return null; + } + + @Override + public void close() { + closed = true; + } + } + + @Test + void contextTrackingAdviceCloseScopeToleratesNullScope() { + assertDoesNotThrow(() -> TomcatServerInstrumentation.ContextTrackingAdvice.closeScope(null)); + } + + @Test + void contextTrackingAdviceCloseScopeClosesNonNullScope() { + RecordingScope scope = new RecordingScope(); + TomcatServerInstrumentation.ContextTrackingAdvice.closeScope(scope); + assertTrue(scope.closed); + } + + @Test + void serviceAdviceCloseScopeToleratesNullScope() { + assertDoesNotThrow(() -> TomcatServerInstrumentation.ServiceAdvice.closeScope(null)); + } + + @Test + void serviceAdviceCloseScopeClosesNonNullScope() { + RecordingScope scope = new RecordingScope(); + TomcatServerInstrumentation.ServiceAdvice.closeScope(scope); + assertTrue(scope.closed); + } +}