Fix StackOverflowError in caused by missing re-entrancy guard#11838
Fix StackOverflowError in caused by missing re-entrancy guard#11838ValentinZakharov wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7e8144fd7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (scope == null) { | ||
| return; | ||
| } | ||
| CallDepthThreadLocalMap.reset(RequestDispatcher.class); |
There was a problem hiding this comment.
Keep the call-depth guard active through context restore
In the normal servlet dispatch path requestContext is non-null because the servlet advice stores DD_CONTEXT_ATTRIBUTE on the request, so resetting the call-depth guard before request.setAttribute(DD_CONTEXT_ATTRIBUTE, requestContext) reopens the same recursion window on exit. With wrappers that redispatch from setAttribute() (the production scenario this fixes), the restore call now starts another fully instrumented forward() while the dispatcher span is still active, which can repeat from each nested exit and still overflow; keep the guard set until after all advice-driven setAttribute calls have completed.
Useful? React with 👍 / 👎.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
What Does This Do
Adds a re-entrancy guard to
RequestDispatcherAdviceso that whenforward()is triggered recursively from insidesetAttribute(), the advice skips immediately instead of running again.Motivation
Production
StackOverflowErrorin SAP Hybris/Spartacus (v1.63.0).When tracing a
RequestDispatcher.forward()call, the tracer injects propagation headers by callingrequest.setAttribute(). In Hybris, a custom request wrapper interceptssetAttribute()and fires anotherforward()- which the tracer instruments again, callingsetAttribute()again:The fix uses
CallDepthThreadLocalMap- already present inAsyncContextInstrumentationfor the same reason - to detect re-entrant calls and exit immediately.Additional Notes
forward()/include()calls made from a servlet body will no longer generate child spans. This is the same trade-off already accepted inAsyncContextInstrumentation.HttpServletRequestWrapper.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]