Skip to content

Fix NPE in NettyResponseChannel when root cause exception has null message #3263

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.

Sign up for GitHub

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

Open
crliao wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix NPE in NettyResponseChannel when root cause exception has null message #3263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions ambry-rest/src/main/java/com/github/ambry/rest/NettyResponseChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,9 +589,12 @@ private FullHttpResponse getErrorResponse(Throwable cause) {
errorResponseStatus = ResponseStatus.getResponseStatus(restServiceErrorCode);
status = getHttpResponseStatus(errorResponseStatus);
if (shouldSendFailureReason(status, restServiceException)) {
errReason = new String(
Utils.getRootCause(cause).getMessage().replaceAll("[\n\t\r]", " ").getBytes(StandardCharsets.US_ASCII),
StandardCharsets.US_ASCII);
String rootMessage = Utils.getRootCause(cause).getMessage();
if (rootMessage != null) {
errReason = new String(
rootMessage.replaceAll("[\n\t\r]", " ").getBytes(StandardCharsets.US_ASCII),

@SophieGuo410 SophieGuo410 May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will Utils.getRootCause(cause) return null

@crliao crliao May 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No — getRootCause(cause) cannot return null here. If the input is non-null, the loop starts with throwable = t (non-null) and only advances while throwable.getCause() != null; when the chain ends it returns the current throwable, still non-null. It only returns null when the input is null. At this call site, cause has already passed the instanceof RestServiceException check on the enclosing if, so it is guaranteed non-null.

@crliao crliao May 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at line 586 if (cause instanceof RestServiceException)

StandardCharsets.US_ASCII);
}
}
if (restServiceException.shouldIncludeExceptionMetadataInResponse()) {
errHeaders = restServiceException.getExceptionHeadersMap();
Expand Down
32 changes: 32 additions & 0 deletions ambry-rest/src/test/java/com/github/ambry/rest/NettyResponseChannelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,27 @@ public void setFailureReasonInResponseTest() throws Exception {
assertFalse("Channel not closed on the server", channel.isActive());
}

/**
* Tests that no NPE occurs when the root cause of a RestServiceException has a null message (e.g.,
* {@link java.util.concurrent.TimeoutException} created by {@link java.util.concurrent.CompletableFuture#orTimeout}).
*/
@Test
public void setFailureReasonNullMessageNoNpeTest() throws Exception {
HttpRequest request =
createRequestWithHeaders(HttpMethod.GET, TestingUri.SetFailureReasonInResponseWithNullMessage.toString());
HttpUtil.setKeepAlive(request, false);

EmbeddedChannel channel = createEmbeddedChannel();
channel.writeInbound(request);

HttpResponse response = channel.readOutbound();
assertEquals(HttpResponseStatus.SERVICE_UNAVAILABLE, response.status());
// No FAILURE_REASON_HEADER when root cause message is null — no NPE should be thrown
assertFalse("Should not have failure reason header when root message is null",
response.headers().contains(NettyResponseChannel.FAILURE_REASON_HEADER));
assertFalse("Channel not closed on the server", channel.isActive());
}

/**
* Sends null input to {@link NettyResponseChannel#setHeader(String, Object)} (through
* {@link MockNettyMessageProcessor}) and tests for reaction.
Expand Down Expand Up @@ -1339,6 +1360,11 @@ enum TestingUri {
* to the client.
*/
SetFailureReasonInResponseWithException,
/**
* When this request is received, a RestServiceException whose root cause has a null message is used on
* onResponseComplete. Verifies no NPE when the root cause message is null.
*/
SetFailureReasonInResponseWithNullMessage,
/**
* Catch all TestingUri.
*/
Expand Down Expand Up @@ -1584,6 +1610,12 @@ private void handleRequest(HttpRequest httpRequest) throws Exception {
request.setArg(RestUtils.InternalKeys.SEND_FAILURE_REASON, Boolean.TRUE);
restResponseChannel.onResponseComplete(new Exception(TestingUri.SetFailureReasonInResponse.toString()));
break;
case SetFailureReasonInResponseWithNullMessage:
request.setArg(RestUtils.InternalKeys.SEND_FAILURE_REASON, Boolean.TRUE);
// TimeoutException() with no message simulates CompletableFuture.orTimeout() behavior
restResponseChannel.onResponseComplete(new RestServiceException("outer message",
new TimeoutException(), RestServiceErrorCode.ServiceUnavailable));
break;
}
}

Expand Down
Loading