RUM-10417: Handle potential StackOverflowError#2990
Conversation
|
🎯 Code Coverage 🔗 Commit SHA: ff8e241 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2990 +/- ##
===========================================
- Coverage 71.26% 71.21% -0.05%
===========================================
Files 859 859
Lines 31449 31457 +8
Branches 5306 5306
===========================================
- Hits 22411 22400 -11
- Misses 7537 7557 +20
+ Partials 1501 1500 -1
🚀 New features to boost your workflow:
|
| tracedRequestListener.onRequestIntercepted(request, span, response, throwable) | ||
| try { | ||
| tracedRequestListener.onRequestIntercepted(request, span, response, throwable) | ||
| } catch (e: StackOverflowError) { |
There was a problem hiding this comment.
While this prevents crashing host app, it still allows having some loops before hitting stack overflow.
Maybe we can even prevent making loops by adding a tag to the Request with a {Interceptor}.toString() or System.identityHashCode (it should be a Set then since the same request may go through multiple interceptors) and reading it back to check if we are in the loop. WDYT?
There was a problem hiding this comment.
One more note: normally Error classes are non-recoverable JVM errors, it is not advised to continue execution when such is thrown.
https://docs.oracle.com/javase/8/docs/api/java/lang/Error.html
An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.
There was a problem hiding this comment.
Not sure we want to make such changes. This PR is mostly to help customer to identify a problem. We want to explain why the stackoverflow error is thrown, not preventing recursive calls. Some customers might like to have them. But I agree that the error should not be swallowed by catch block and should be re-thrown.
There was a problem hiding this comment.
I have added the re-throwing of the error.
There was a problem hiding this comment.
We can indeed re-throw it. My point is that after getting such error it is not possible to continue normal execution, because stack may not recover to the state (free capacity) enough to continue reliably. Even if may continue here, if stack is not unrolled, similar error may pop up in random place further.
36741e6 to
ff8e241
Compare
What does this PR do?
Wraps the
onRequestInterceptedmethod inTracingInterceptor.ktinside atry - catchblock that logs an error in case aStackOverflowErroris caught.Motivation
If a customer implements a custom
TracedRequestListenerand keeps retrying a request withonRequestIntercepted, this will run forever, eventually leading to aStackOverflowError.Review checklist (to be filled by reviewers)