-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RemoteException.getMessage includes SerializableError parameters #633
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
type: improvement | ||
improvement: | ||
description: RemoteException.getMessage (unsafe message) includes SerializableError parameters. RemoteException.getLogMessage (safe) is not impacted. | ||
links: | ||
- https://github.com/palantir/conjure-java-runtime-api/pull/633 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,11 +26,13 @@ | |
public final class RemoteException extends RuntimeException implements SafeLoggable { | ||
private static final long serialVersionUID = 1L; | ||
|
||
private final String message; | ||
private final String stableMessage; | ||
private final SerializableError error; | ||
private final int status; | ||
private final List<Arg<?>> args; | ||
// Lazily evaluated based on the stableMessage, errorInstanceId, and args. | ||
@SuppressWarnings("MutableException") | ||
private String unsafeMessage; | ||
|
||
/** Returns the error thrown by a remote process which caused an RPC call to fail. */ | ||
public SerializableError getError() { | ||
|
@@ -44,17 +46,40 @@ public int getStatus() { | |
|
||
public RemoteException(SerializableError error, int status) { | ||
this.stableMessage = error.errorCode().equals(error.errorName()) | ||
? String.format("RemoteException: %s", error.errorCode()) | ||
: String.format("RemoteException: %s (%s)", error.errorCode(), error.errorName()); | ||
this.message = this.stableMessage + " with instance ID " + error.errorInstanceId(); | ||
? "RemoteException: " + error.errorCode() | ||
: "RemoteException: " + error.errorCode() + " (" + error.errorName() + ")"; | ||
this.error = error; | ||
this.status = status; | ||
this.args = Collections.singletonList(SafeArg.of("errorInstanceId", error.errorInstanceId())); | ||
} | ||
|
||
@Override | ||
public String getMessage() { | ||
return message; | ||
// This field is not used in most environments so the cost of computation may be avoided. | ||
String messageValue = unsafeMessage; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious, why do we need the if (unsafeMessage == null) {
unsafeMessage = renderUnsafeMessage();
}
return unsafeMessage; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optimization, not likely a measurable difference, but we’re reading the field value once instead of twice. I find it cleaner to assert on the same value that’s returned as well (avoids a class of race condition bugs) |
||
if (messageValue == null) { | ||
messageValue = renderUnsafeMessage(); | ||
unsafeMessage = messageValue; | ||
} | ||
return messageValue; | ||
} | ||
|
||
private String renderUnsafeMessage() { | ||
StringBuilder builder = new StringBuilder() | ||
.append(stableMessage) | ||
.append(" with instance ID ") | ||
.append(error.errorInstanceId()); | ||
if (!error.parameters().isEmpty()) { | ||
builder.append(": {"); | ||
error.parameters() | ||
.forEach((name, unsafeValue) -> | ||
builder.append(name).append('=').append(unsafeValue).append(", ")); | ||
// remove the trailing space | ||
builder.setLength(builder.length() - 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Joiner? Does that allocate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case it's unlikely that the last character resulted in a buffer expansion, so reducing the length by 1 is a non-volatile write to the StringBuilder.count integer. Joiner would work nicely if this didn't involve relatively complex prefix parameters -- we'd end up creating a bunch of intermediate strings. |
||
// replace the trailing comma with a close curly brace | ||
builder.setCharAt(builder.length() - 1, '}'); | ||
} | ||
return builder.toString(); | ||
} | ||
|
||
@Override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add the new safety annotations to this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t hurt, although we already consider throwable.getMessage unsafe across the board