Fix StackOverflowError in caused by missing re-entrancy guard#11838
Conversation
ValentinZakharov
added
type: bug fix
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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.
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 👍 / 👎.
There was a problem hiding this comment.
this makes sense to me, and even if it's not the case, I think it's safer to reset the depth count at the very end of this method to avoid mistakes if the body changes.
There was a problem hiding this comment.
I think moving it in a try finally after the setAttribute just below is safer
🟢 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. |
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
Makes sense to me. I also noticed few errors like that in the telemetry that hopefully will be now fixed. The codex comment looks valid and should be addressed
ValentinZakharov
force-pushed
the
vzakharov/fix-request-dispatcher-reentrancy-soe
branch
from
f7e8144 to
cb6c968
Compare
|
🎯 Code Coverage (details) 🔗 Commit SHA: cb6c968 | Docs | Datadog PR Page | Give us feedback! |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
vzakharov/fix-request-dispatcher-reentrancy-soe
branch
…e-entrancy guard (#11838) Fix StackOverflowError in RequestDispatcherAdvice caused by missing re-entrancy guard Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
3 participants
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]