Fix DDSpan.addThrowable crashing when exception getMessage() throws#11428
Conversation
This comment has been minimized.
This comment has been minimized.
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
looks ok. I left some comments - You might need to run spotless to fix the formatting
| * Safely retrieves the message from a throwable. Returns {@code null} if {@code t} is {@code | ||
| * null}, or a diagnostic string of the form {@code "(Exception message unavailable for ClassName: | ||
| * getMessage() threw ExceptionType)"} if {@link Throwable#getMessage()} itself throws. | ||
| * Third-party exception classes occasionally use formatting utilities (e.g. {@code | ||
| * java.text.MessageFormat}) inside {@code getMessage()}, which can throw when the pattern | ||
| * contains non-integer placeholders. | ||
| */ |
There was a problem hiding this comment.
That can be formatted as a standard javadoc. i.e.
@param ...
@return ...
There was a problem hiding this comment.
Fixed, thanks
| String cutMessage = "\t... trace centre-cut to " + maxChars + " chars ..."; | ||
| int retainedLength = maxChars - cutMessage.length() - 2; // 2 for the newlines | ||
| if (retainedLength <= 0) { | ||
| return cutMessage + System.lineSeparator(); |
There was a problem hiding this comment.
this looks added so will modify also the existing implementation
There was a problem hiding this comment.
This is the fix for edge case: when maxChars is smaller than cutMessage itself, retainedLength goes negative and substring() throws. Caught it while working in this area
| t.getClass().getName() | ||
| + System.lineSeparator() | ||
| + Arrays.stream(t.getStackTrace()) | ||
| .map(f -> "\tat " + f) | ||
| .collect(Collectors.joining(System.lineSeparator())); |
There was a problem hiding this comment.
For precision the original implementation also prints the stack for suppressed and cause. I think that, for simplicity, this will be ok.
| } catch (Exception ignored) { | ||
| // printStackTrace() failed (e.g. getMessage() throws inside toString()). | ||
| // Reconstruct from getStackTrace() so the call site is still locatable. | ||
| trace = |
There was a problem hiding this comment.
We do not know if getStackTrace can also throw in some cases? I would also protect inside a try catch
There was a problem hiding this comment.
Good catch! Fixed! Thanks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
vzakharov/fix-addthrowable-broken-getmessage
branch
2 participants
What Does This Do
Adds
StackTraces.safeGetMessage(Throwable)and uses it inDDSpan.addThrowablewheregetMessage()was called directly. Also guardsStackTraces.getStackTrace()which callsgetMessage()indirectly viaprintStackTrace()→toString().# Motivation
A third-party exception's
getMessage()can throw if it internally usesMessageFormat.format()with non-integer placeholders (e.g.{TotalMilliseconds}).MessageFormattries to parse the placeholder as an argument index viaInteger.parseInt, which throwsNumberFormatException→IllegalArgumentExceptionBefore the fix:
After the fix:
Additional Notes
safeGetMessageusescatch (Exception e)intentionally -Errorsubtypes (OutOfMemoryError,StackOverflowError) are not caught as masking them would cause more harm than good.printStackTrace()fails, the fallback captures frames of the top-level exception viagetStackTrace()[]. TheCaused by:chain is lost in this edge case - acceptable given how rarelygetMessage()throws.ERROR_TYPEandERROR_MSG(fallback string) are always recorded, giving enough signal to identify the problem even without a full stack trace.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 issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.