Skip to content

Guard Tomcat CoyoteAdapter advice against null-scope NPE - #12375

Draft
dougqh wants to merge 1 commit into
masterfrom
dougqh/fix-tomcat-coyoteadapter-npe
Draft

Guard Tomcat CoyoteAdapter advice against null-scope NPE#12375
dougqh wants to merge 1 commit into
masterfrom
dougqh/fix-tomcat-coyoteadapter-npe

Conversation

@dougqh

@dougqh dougqh commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

What Does This Do

Guards the Tomcat CoyoteAdapter server advice against a null-scope NullPointerException that can occur if the paired enter-advice fails before assigning its @Advice.Local ContextScope.

TomcatServerInstrumentation.ContextTrackingAdvice.closeScope and TomcatServerInstrumentation.ServiceAdvice.closeScope both called .close() unconditionally on an @Advice.Local ContextScope. Both are guarded with if (scope != null) now.

Motivation

Error Tracking issue: https://app.datadoghq.com/error-tracking/issue/5ce30218-b43b-11f0-ac08-da7ad0900002

java.lang.NullPointerException
  at (redacted: 6 frames)
  at java.base/java.lang.VirtualThread.run(VirtualThread.java:460)

Logged via the agent's own suppressed-exception mechanism as:

Failed to handle exception in instrumentation for org.apache.catalina.connector.CoyoteAdapter

ContextTrackingAdvice.extractParent and ServiceAdvice.onService are both @Advice.OnMethodEnter(suppress = Throwable.class). If either throws before assigning its @Advice.Local ContextScope (e.g. DECORATE.extract(req), DECORATE.startSpan(...), or a request-attribute call throwing inside an app's custom Request/attribute wrapper), the exception is silently swallowed by ByteBuddy's suppression bytecode. CoyoteAdapter.service() then proceeds normally, and the paired @Advice.OnMethodExit closeScope unconditionally calls .close() on the still-null local — a second NPE that is also swallowed and logged, fully masking the original failure. Because both exceptions are handled by generated advice bytecode rather than real application frames, the visible stack trace is almost entirely redacted, which is why this surfaces as a bare VirtualThread.run frame with no other context.

A related but narrower instance of this same failure mode (parentContext == null specifically) was already fixed in #11968 (rootContext() fallback), but the underlying structural issue — closeScope not tolerating a null local — was never addressed, so any other exception in the enter-advice reproduces the same NPE.

Additional Notes

Added a direct JUnit 5 unit test (TomcatServerInstrumentationTest) calling the two closeScope advice methods with null and with a real ContextScope, since both are plain public static methods and ContextScope is an interface — no bytecode instrumentation harness or mocking framework needed.

Contributor Checklist

  • Add an entry in docs/release_notes.md if this change affects the user-facing functionality. — N/A, internal fix.
  • Verify code coverage of new lines
  • Confirm this change does not add flakiness in tests

Jira ticket: [PROJ-IDENT]

If extractParent()/onService() throws before assigning its @Advice.Local
ContextScope, the throwable is swallowed (suppress = Throwable.class), and
the paired closeScope() then NPEs on the still-null local, masking the real
failure and producing a misleading "Failed to handle exception in
instrumentation for CoyoteAdapter" log entry.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dougqh dougqh added type: bug fix Bug fix inst: others All other instrumentations tag: ai generated Largely based on code generated by an AI or LLM labels Sep 2, 2026
@datadog-datadog-prod-us1

Copy link
Copy Markdown
Contributor

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 69.74% (+10.72%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: bcad0c3 | Docs | View more details | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 14.76 s 14.62 s [+0.1%; +1.7%] (maybe worse)
startup:insecure-bank:tracing:Agent 13.67 s 13.75 s [-1.6%; +0.4%] (no difference)
startup:petclinic:appsec:Agent 16.49 s 16.65 s [-5.5%; +3.5%] (no difference)
startup:petclinic:iast:Agent 16.82 s 16.95 s [-1.5%; +0.1%] (no difference)
startup:petclinic:profiling:Agent 16.75 s 16.81 s [-1.6%; +0.9%] (no difference)
startup:petclinic:sca:Agent 16.86 s 16.84 s [-0.8%; +1.1%] (no difference)
startup:petclinic:tracing:Agent 15.95 s 15.66 s [-2.5%; +6.1%] (no difference)

Commit: bcad0c3f · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: others All other instrumentations tag: ai generated Largely based on code generated by an AI or LLM type: bug fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant