From 77ffe7621212be9f462ca37a542a13d167eca4e0 Mon Sep 17 00:00:00 2001
From: Tsuyoshi Ozawa <ozawa@apache.org>
Date: Wed, 13 Jul 2016 21:28:04 +0900
Subject: [PATCH] HADOOP-11361. Fix a race condition in
 MetricsSourceAdapter.updateJmxCache. Contributed by Vinayakumar B, Yongjun
 Zhang, and Brahma Reddy Battula. (ozawa)

---
 .../metrics2/impl/MetricsSourceAdapter.java   | 33 ++++++++-----------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
index cbba0148264..3406aceb0a2 100644
--- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
+++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
@@ -31,6 +31,7 @@ import javax.management.ReflectionException;
 
 import static com.google.common.base.Preconditions.*;
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.Maps;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -59,7 +60,6 @@ class MetricsSourceAdapter implements DynamicMBean {
   private final MBeanInfoBuilder infoBuilder;
   private final Iterable<MetricsTag> injectedTags;
 
-  private Iterable<MetricsRecordImpl> lastRecs;
   private boolean lastRecsCleared;
   private long jmxCacheTS = 0;
   private long jmxCacheTTL;
@@ -175,18 +175,19 @@ class MetricsSourceAdapter implements DynamicMBean {
       }
     }
 
+    // HADOOP-11361: Release lock here for avoid deadlock between
+    // MetricsSystemImpl's lock and MetricsSourceAdapter's lock.
+    Iterable<MetricsRecordImpl> lastRecs = null;
     if (getAllMetrics) {
-      MetricsCollectorImpl builder = new MetricsCollectorImpl();
-      getMetrics(builder, true);
+      lastRecs = getMetrics(new MetricsCollectorImpl(), true);
     }
 
-    synchronized(this) {
-      updateAttrCache();
-      if (getAllMetrics) {
-        updateInfoCache();
+    synchronized (this) {
+      if (lastRecs != null) {
+        updateAttrCache(lastRecs);
+        updateInfoCache(lastRecs);
       }
       jmxCacheTS = Time.now();
-      lastRecs = null;  // in case regular interval update is not running
       lastRecsCleared = true;
     }
   }
@@ -194,11 +195,6 @@ class MetricsSourceAdapter implements DynamicMBean {
   Iterable<MetricsRecordImpl> getMetrics(MetricsCollectorImpl builder,
                                          boolean all) {
     builder.setRecordFilter(recordFilter).setMetricFilter(metricFilter);
-    synchronized(this) {
-      if (lastRecs == null && jmxCacheTS == 0) {
-        all = true; // Get all the metrics to populate the sink caches
-      }
-    }
     try {
       source.getMetrics(builder, all);
     } catch (Exception e) {
@@ -209,10 +205,7 @@ class MetricsSourceAdapter implements DynamicMBean {
         rb.add(t);
       }
     }
-    synchronized(this) {
-      lastRecs = builder.getRecords();
-      return lastRecs;
-    }
+    return builder.getRecords();
   }
 
   synchronized void stop() {
@@ -246,13 +239,15 @@ class MetricsSourceAdapter implements DynamicMBean {
     return jmxCacheTTL;
   }
 
-  private void updateInfoCache() {
+  private void updateInfoCache(Iterable<MetricsRecordImpl> lastRecs) {
+    Preconditions.checkNotNull(lastRecs, "LastRecs should not be null");
     LOG.debug("Updating info cache...");
     infoCache = infoBuilder.reset(lastRecs).get();
     LOG.debug("Done");
   }
 
-  private int updateAttrCache() {
+  private int updateAttrCache(Iterable<MetricsRecordImpl> lastRecs) {
+    Preconditions.checkNotNull(lastRecs, "LastRecs should not be null");
     LOG.debug("Updating attr cache...");
     int recNo = 0;
     int numMetrics = 0;