Skip to content

Add null-safe ContextScope.close(scope) helper - #12377

Draft
dougqh wants to merge 1 commit into
masterfrom
dougqh/context-scope-null-safe-close
Draft

Add null-safe ContextScope.close(scope) helper#12377
dougqh wants to merge 1 commit into
masterfrom
dougqh/context-scope-null-safe-close

Conversation

@dougqh

@dougqh dougqh commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

What Does This Do

Adds a static, null-safe ContextScope.close(ContextScope scope) helper that no-ops on null instead of throwing.

Motivation

While fixing #12375 (a NullPointerException in Tomcat's CoyoteAdapter advice), I found the underlying shape of that bug is a widespread, copy-pasted pattern across instrumentation advice:

  • An @Advice.OnMethodEnter(suppress = Throwable.class) method attaches a ContextScope (or the older AgentScope) and assigns it to an @Advice.Local/@Advice.Enter variable, but the attach happens after other code that can throw (a decorator call, a request-attribute lookup, etc).
  • If that enter-advice throws, the exception is silently swallowed by ByteBuddy's suppression bytecode, the instrumented method proceeds to run normally, and the local is left null.
  • The paired @Advice.OnMethodExit then calls .close() on that (still-null) local unconditionally, producing a second, masking NullPointerException that's also swallowed and logged, hiding the real root cause entirely.

A repo-wide search turned up 40+ advice classes across 25+ instrumented libraries with this exact unguarded shape — including all 5 Jetty server versions, Spring MVC's RenderAdvice, Spring WebFlux's DispatcherHandlerAdvice, and the generic @Trace/OpenTelemetry @WithSpan annotation advice used across user-code method tracing. Some advice classes already null-check correctly (Undertow, Servlet, gRPC, Play, Liberty, and — inconsistently — some sibling advice classes in the very same files as the unguarded ones), so this isn't a case of the pattern being unknown, just inconsistently applied.

Additional Notes

This PR only adds the shared helper, as a starting point for discussion — it does not sweep the 40+ call sites yet. That's intentionally left for follow-up PRs (either a phased sweep, or converting sites opportunistically as they're touched) pending team input on the right rollout approach.

ContextScope lives in components/context, a low-level module shared across the whole tracer, so this fix (and any future call-site conversions) benefits every instrumentation, not just one.

Contributor Checklist

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

Jira ticket: [PROJ-IDENT]

Many advice classes attach a ContextScope on enter and close it
unconditionally on exit; if the enter advice throws before assigning the
scope (its exception typically suppressed by suppress = Throwable.class),
the exit advice NPEs on the null scope, masking the real failure. This adds
a static helper that call sites can use instead of scope.close() to avoid
that class of bug without a null check at every site.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dougqh dougqh added type: feature Enhancements and improvements comp: context propagation Trace context propagation tag: ai generated Largely based on code generated by an AI or LLM labels Sep 2, 2026
@datadog-datadog-prod-us1

This comment has been minimized.

@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.81 s 14.69 s [-0.1%; +1.8%] (no difference)
startup:insecure-bank:tracing:Agent 13.61 s 13.74 s [-2.1%; +0.1%] (no difference)
startup:petclinic:appsec:Agent 17.58 s 17.27 s [+1.0%; +2.6%] (significantly worse)
startup:petclinic:iast:Agent 16.83 s 17.59 s [-8.5%; -0.2%] (maybe better)
startup:petclinic:profiling:Agent 17.41 s 17.37 s [-1.2%; +1.6%] (no difference)
startup:petclinic:sca:Agent 17.62 s 17.48 s [-0.3%; +2.0%] (no difference)
startup:petclinic:tracing:Agent 16.70 s 16.55 s [-0.2%; +2.0%] (no difference)

Commit: e1179e9d · 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.

@mcculls

mcculls commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

+1 for adding this helper, and for consistency across instrumentations

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

Labels

comp: context propagation Trace context propagation tag: ai generated Largely based on code generated by an AI or LLM type: feature Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants