From d6b2257655643fb9d44e340a930398463c68a3c8 Mon Sep 17 00:00:00 2001 From: Xiaolin Ha Date: Wed, 18 Aug 2021 13:28:46 +0800 Subject: [PATCH] HBASE-26087 JVM crash when displaying RPC params by MonitoredRPCHandler (#3489) Signed-off-by: stack --- .../monitoring/MonitoredRPCHandlerImpl.java | 15 +++- .../hbase/monitoring/TestTaskMonitor.java | 71 ++++++++++++++++++- 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java index d194d10186f..aca3eeba0b1 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java @@ -22,10 +22,10 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.Map; -import org.apache.yetus.audience.InterfaceAudience; import org.apache.hadoop.hbase.client.Operation; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.yetus.audience.InterfaceAudience; import org.apache.hbase.thirdparty.com.google.protobuf.Message; /** @@ -43,6 +43,8 @@ public class MonitoredRPCHandlerImpl extends MonitoredTaskImpl private String methodName = ""; private Object [] params = {}; private Message packet; + private boolean snapshot = false; + private Map callInfoMap = new HashMap<>(); public MonitoredRPCHandlerImpl() { super(); @@ -53,11 +55,14 @@ public class MonitoredRPCHandlerImpl extends MonitoredTaskImpl @Override public synchronized MonitoredRPCHandlerImpl clone() { - return (MonitoredRPCHandlerImpl) super.clone(); + MonitoredRPCHandlerImpl clone = (MonitoredRPCHandlerImpl) super.clone(); + clone.callInfoMap = generateCallInfoMap(); + clone.snapshot = true; + return clone; } /** - * Gets the status of this handler; if it is currently servicing an RPC, + * Gets the status of this handler; if it is currently servicing an RPC, * this status will include the RPC information. * @return a String describing the current status. */ @@ -233,6 +238,10 @@ public class MonitoredRPCHandlerImpl extends MonitoredTaskImpl @Override public synchronized Map toMap() { + return this.snapshot ? this.callInfoMap : generateCallInfoMap(); + } + + private Map generateCallInfoMap() { // only include RPC info if the Handler is actively servicing an RPC call Map map = super.toMap(); if (getState() != State.RUNNING) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java index 1145131affe..c9a9fc98069 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java @@ -17,10 +17,13 @@ */ package org.apache.hadoop.hbase.monitoring; -import static org.junit.Assert.*; - +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import java.util.ArrayList; import java.util.List; +import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HBaseClassTestRule; @@ -34,9 +37,12 @@ import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; @Category({MiscTests.class, SmallTests.class}) public class TestTaskMonitor { + private static final Logger LOG = LoggerFactory.getLogger(TestTaskMonitor.class); @ClassRule public static final HBaseClassTestRule CLASS_RULE = @@ -226,5 +232,66 @@ public class TestTaskMonitor { assertEquals("status3", task.getStatusJournal().get(1).getStatus()); tm.shutdown(); } + + @Test + public void testClone() throws Exception { + MonitoredRPCHandlerImpl monitor = new MonitoredRPCHandlerImpl(); + monitor.abort("abort RPC"); + TestParam testParam = new TestParam("param1"); + monitor.setRPC("method1", new Object[]{ testParam }, 0); + MonitoredRPCHandlerImpl clone = monitor.clone(); + assertEquals(clone.getDescription(), monitor.getDescription()); + assertEquals(clone.getState(), monitor.getState()); + assertEquals(clone.getStatus(), monitor.getStatus()); + assertEquals(clone.toString(), monitor.toString()); + assertEquals(clone.toMap(), monitor.toMap()); + assertEquals(clone.toJSON(), monitor.toJSON()); + + // mark complete and make param dirty + monitor.markComplete("complete RPC"); + testParam.setParam("dirtyParam"); + assertEquals(clone.getDescription(), monitor.getDescription()); + assertNotEquals(clone.getState(), monitor.getState()); + assertNotEquals(clone.getStatus(), monitor.getStatus()); + monitor.setState(MonitoredTask.State.RUNNING); + try { + // when markComplete, the param in monitor is set null, so toMap should fail here + monitor.toMap(); + fail("Should not call toMap successfully, because param=null"); + } catch (Exception e) { + } + // the param of clone monitor should not be dirty + assertNotEquals("[dirtyString]", + String.valueOf(((Map) clone.toMap().get("rpcCall")).get("params"))); + + monitor.resume("resume"); + monitor.setRPC("method2", new Object[]{new TestParam("param2")}, 1); + assertNotEquals(((Map) clone.toMap().get("rpcCall")).get("params"), + ((Map) monitor.toMap().get("rpcCall")).get( + "params")); + LOG.info(String.valueOf(clone.toMap())); + LOG.info(String.valueOf(monitor.toMap())); + assertNotEquals(clone.toString(), monitor.toString()); + assertNotEquals(clone.getRPCQueueTime(), monitor.getRPCQueueTime()); + assertNotEquals(clone.toMap(), monitor.toMap()); + assertNotEquals(clone.toJSON(), monitor.toJSON()); + } + + private class TestParam { + public String param = null; + + public TestParam(String param) { + this.param = param; + } + + public void setParam(String param) { + this.param = param; + } + + @Override + public String toString() { + return param; + } + } }