This is an automated email from the ASF dual-hosted git repository.
cconnell pushed a commit to branch branch-2.6
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-2.6 by this push:
new 427f62d12dc HBASE-28589: ServerCall.setResponse swallows IOException
and leaves client without response (#7156)
427f62d12dc is described below
commit 427f62d12dc74b78f31ecabca2b4d19baf74c800
Author: ZHENYU LI <[email protected]>
AuthorDate: Tue Jul 22 10:12:38 2025 -0400
HBASE-28589: ServerCall.setResponse swallows IOException and leaves client
without response (#7156)
When IOException occurs during response creation in
ServerCall.setResponse(),
the method only logs a warning and sets response to null. This causes client
to receive no response or experience connection issues without knowing what
went wrong on server side.
This patch:
- Catches IOException during response creation
- Creates an error response to send back to client
- Handles the case where even error response creation fails
- Adds unit tests to verify the behavior
Signed-off by: Duo Zhang <[email protected]>
Signed-off by: Charles Connell <[email protected]>
---
.../org/apache/hadoop/hbase/ipc/ServerCall.java | 27 ++++
.../apache/hadoop/hbase/ipc/TestServerCall.java | 174 +++++++++++++++++++++
2 files changed, 201 insertions(+)
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerCall.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerCall.java
index 8d8d443aa12..db181d6d6f3 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerCall.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerCall.java
@@ -339,6 +339,7 @@ public abstract class ServerCall<T extends
ServerRpcConnection> implements RpcCa
bc = new BufferChain(responseBufs);
} catch (IOException e) {
RpcServer.LOG.warn("Exception while creating response " + e);
+ bc = createFallbackErrorResponse(e);
}
this.response = bc;
// Once a response message is created and set to this.response, this Call
can be treated as
@@ -375,6 +376,32 @@ public abstract class ServerCall<T extends
ServerRpcConnection> implements RpcCa
headerBuilder.setException(exceptionBuilder.build());
}
+ /*
+ * Creates a fallback error response when the primary response creation
fails. This method is
+ * invoked as a last resort when an IOException occurs during the normal
response creation
+ * process. It attempts to create a minimal error response containing only
the error information,
+ * without any cell blocks or additional data. The purpose is to ensure that
the client receives
+ * some indication of the failure rather than experiencing a silent
connection drop. This provides
+ * better error handling on the client side.
+ */
+ private BufferChain createFallbackErrorResponse(IOException
originalException) {
+ try {
+ ResponseHeader.Builder headerBuilder = ResponseHeader.newBuilder();
+ headerBuilder.setCallId(this.id);
+ String responseErrorMsg =
+ "Failed to create response due to: " + originalException.getMessage();
+ setExceptionResponse(originalException, responseErrorMsg, headerBuilder);
+ Message header = headerBuilder.build();
+ ByteBuffer headerBuf = createHeaderAndMessageBytes(null, header, 0,
null);
+ this.isError = true;
+ return new BufferChain(new ByteBuffer[] { headerBuf });
+ } catch (IOException e) {
+ RpcServer.LOG.error("Failed to create error response for client,
connection may be dropped",
+ e);
+ return null;
+ }
+ }
+
static ByteBuffer createHeaderAndMessageBytes(Message result, Message
header, int cellBlockSize,
List<ByteBuffer> cellBlock) throws IOException {
// Organize the response as a set of bytebuffers rather than collect it
all together inside
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestServerCall.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestServerCall.java
new file mode 100644
index 00000000000..46239e95859
--- /dev/null
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestServerCall.java
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.ipc;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.net.InetAddress;
+import java.nio.ByteBuffer;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.CellScanner;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.codec.Codec;
+import org.apache.hadoop.hbase.io.ByteBuffAllocator;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.apache.hadoop.hbase.testclassification.RPCTests;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.protobuf.BlockingService;
+import
org.apache.hbase.thirdparty.com.google.protobuf.Descriptors.MethodDescriptor;
+import org.apache.hbase.thirdparty.com.google.protobuf.Message;
+
+import org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos;
+import
org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos.RequestHeader;
+
+/**
+ * Test for ServerCall IOException handling in setResponse method.
+ */
+@Category({ RPCTests.class, MediumTests.class })
+public class TestServerCall {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestServerCall.class);
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestServerCall.class);
+
+ private Configuration conf;
+ private NettyServerRpcConnection mockConnection;
+ private RequestHeader header;
+ private Message mockParam;
+ private ByteBuffAllocator mockAllocator;
+ private CellBlockBuilder mockCellBlockBuilder;
+ private InetAddress mockAddr;
+ private BlockingService mockService;
+ private MethodDescriptor mockMethodDescriptor;
+
+ @Before
+ public void setUp() throws Exception {
+ conf = HBaseConfiguration.create();
+ mockConnection = mock(NettyServerRpcConnection.class);
+ header =
RequestHeader.newBuilder().setCallId(1).setMethodName("testMethod")
+ .setRequestParam(true).build();
+ mockParam = mock(Message.class);
+ mockAllocator = mock(ByteBuffAllocator.class);
+ mockCellBlockBuilder = mock(CellBlockBuilder.class);
+ mockAddr = mock(InetAddress.class);
+
+ mockMethodDescriptor =
+
org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.AdminService.getDescriptor()
+ .getMethods().get(0);
+
+ mockService = mock(BlockingService.class);
+ mockConnection.codec = mock(Codec.class);
+
+ when(mockAllocator.isReservoirEnabled()).thenReturn(false);
+ }
+
+ /**
+ * Test that when IOException occurs during response creation in
setResponse, an error response is
+ * created and sent to the client instead of leaving the response as null.
+ */
+ @Test
+ public void testSetResponseWithIOException() throws Exception {
+ // Create a CellBlockBuilder that throws IOException
+ CellBlockBuilder failingCellBlockBuilder = mock(CellBlockBuilder.class);
+ doThrow(new IOException("Test IOException during
buildCellBlock")).when(failingCellBlockBuilder)
+ .buildCellBlock(any(), any(), any());
+
+ // Create NettyServerCall instance
+ NettyServerCall call = new NettyServerCall(1, mockService,
mockMethodDescriptor, header,
+ mockParam, null, mockConnection, 100, mockAddr,
System.currentTimeMillis(), 60000,
+ mockAllocator, failingCellBlockBuilder, null);
+
+ // Set a successful response, but CellBlockBuilder will fail
+ Message mockResponse = mock(Message.class);
+ CellScanner mockCellScanner = mock(CellScanner.class);
+
+ LOG.info("Testing setResponse with IOException in buildCellBlock");
+ call.setResponse(mockResponse, mockCellScanner, null, null);
+
+ // Verify that response is not null and contains error information
+ BufferChain response = call.getResponse();
+ assertNotNull("Response should not be null even when IOException occurs",
response);
+ assertTrue("Call should be marked as error", call.isError);
+
+ // Verify the response buffer is valid
+ ByteBuffer[] bufs = response.getBuffers();
+ assertNotNull("Response buffers should not be null", bufs);
+ assertTrue("Response should have at least one buffer", bufs.length > 0);
+ }
+
+ /**
+ * Test the case where both normal response creation and error response
creation fail with
+ * IOException.
+ */
+ @Test
+ public void testSetResponseWithDoubleIOException() throws Exception {
+
+ CellBlockBuilder failingCellBlockBuilder = mock(CellBlockBuilder.class);
+ doThrow(new IOException("Test
IOException")).when(failingCellBlockBuilder).buildCellBlock(any(),
+ any(), any());
+
+ NettyServerCall call = new NettyServerCall(1, mockService,
mockMethodDescriptor, header,
+ mockParam, null, mockConnection, 100, mockAddr,
System.currentTimeMillis(), 60000,
+ mockAllocator, failingCellBlockBuilder, null);
+
+ Message mockResponse = mock(Message.class);
+ CellScanner mockCellScanner = mock(CellScanner.class);
+
+ // Even if error response creation might fail, the call should still be
marked as error
+ call.setResponse(mockResponse, mockCellScanner, null, null);
+ assertTrue("Call should be marked as error", call.isError);
+ }
+
+ /**
+ * Test normal response creation to ensure our changes don't break the
normal flow.
+ */
+ @Test
+ public void testSetResponseNormalFlow() throws Exception {
+ CellBlockBuilder normalCellBlockBuilder = mock(CellBlockBuilder.class);
+ when(normalCellBlockBuilder.buildCellBlock(any(), any(),
any())).thenReturn(null);
+
+ NettyServerCall call = new NettyServerCall(1, mockService,
mockMethodDescriptor, header,
+ mockParam, null, mockConnection, 100, mockAddr,
System.currentTimeMillis(), 60000,
+ mockAllocator, normalCellBlockBuilder, null);
+
+ RPCProtos.CellBlockMeta mockResponse =
+ RPCProtos.CellBlockMeta.newBuilder().setLength(0).build();
+
+ LOG.info("Testing normal setResponse flow");
+ call.setResponse(mockResponse, null, null, null);
+
+ BufferChain response = call.getResponse();
+ assertNotNull("Response should not be null in normal flow", response);
+ assertTrue("Call should not be marked as error in normal flow",
!call.isError);
+ }
+}
