[hbase] branch branch-2 updated: HBASE-23356 When construct StoreScanner throw exceptions it is possible to left some KeyValueScanner not closed. (#891)

[bi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

binlijin pushed a commit to branch branch-2
in repository https://gitbox.apache.org/repos/asf/hbase.git


The following commit(s) were added to refs/heads/branch-2 by this push:
     new 2e8ea9e  HBASE-23356 When construct StoreScanner throw exceptions it is possible to left some KeyValueScanner not closed. (#891)
2e8ea9e is described below

commit 2e8ea9e2720b3e2bf114d12c5f68b1b5f9ff2b34
Author: binlijin <bi...@gmail.com>
AuthorDate: Wed Dec 4 10:34:07 2019 +0800

    HBASE-23356 When construct StoreScanner throw exceptions it is possible to left some KeyValueScanner not closed. (#891)
    
    Signed-off-by: GuangxuCheng  <gu...@gmail.com>
---
 .../apache/hadoop/hbase/regionserver/HStore.java   | 60 +++++++++++++++-------
 .../hadoop/hbase/regionserver/StoreScanner.java    |  7 ++-
 2 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
index 8406ec8..c0bd0dd 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
@@ -1270,18 +1270,34 @@ public class HStore implements Store, HeapSize, StoreConfigInformation, Propagat
       this.lock.readLock().unlock();
     }
 
-    // First the store file scanners
+    try {
+      // First the store file scanners
+
+      // TODO this used to get the store files in descending order,
+      // but now we get them in ascending order, which I think is
+      // actually more correct, since memstore get put at the end.
+      List<StoreFileScanner> sfScanners = StoreFileScanner
+        .getScannersForStoreFiles(storeFilesToScan, cacheBlocks, usePread, isCompaction, false,
+          matcher, readPt);
+      List<KeyValueScanner> scanners = new ArrayList<>(sfScanners.size() + 1);
+      scanners.addAll(sfScanners);
+      // Then the memstore scanners
+      scanners.addAll(memStoreScanners);
+      return scanners;
+    } catch (Throwable t) {
+      clearAndClose(memStoreScanners);
+      throw t instanceof IOException ? (IOException) t : new IOException(t);
+    }
+  }
 
-    // TODO this used to get the store files in descending order,
-    // but now we get them in ascending order, which I think is
-    // actually more correct, since memstore get put at the end.
-    List<StoreFileScanner> sfScanners = StoreFileScanner.getScannersForStoreFiles(storeFilesToScan,
-      cacheBlocks, usePread, isCompaction, false, matcher, readPt);
-    List<KeyValueScanner> scanners = new ArrayList<>(sfScanners.size() + 1);
-    scanners.addAll(sfScanners);
-    // Then the memstore scanners
-    scanners.addAll(memStoreScanners);
-    return scanners;
+  private static void clearAndClose(List<KeyValueScanner> scanners) {
+    if (scanners == null) {
+      return;
+    }
+    for (KeyValueScanner s : scanners) {
+      s.close();
+    }
+    scanners.clear();
   }
 
   /**
@@ -1335,15 +1351,21 @@ public class HStore implements Store, HeapSize, StoreConfigInformation, Propagat
         this.lock.readLock().unlock();
       }
     }
-    List<StoreFileScanner> sfScanners = StoreFileScanner.getScannersForStoreFiles(files,
-      cacheBlocks, usePread, isCompaction, false, matcher, readPt);
-    List<KeyValueScanner> scanners = new ArrayList<>(sfScanners.size() + 1);
-    scanners.addAll(sfScanners);
-    // Then the memstore scanners
-    if (memStoreScanners != null) {
-      scanners.addAll(memStoreScanners);
+    try {
+      List<StoreFileScanner> sfScanners = StoreFileScanner
+        .getScannersForStoreFiles(files, cacheBlocks, usePread, isCompaction, false, matcher,
+          readPt);
+      List<KeyValueScanner> scanners = new ArrayList<>(sfScanners.size() + 1);
+      scanners.addAll(sfScanners);
+      // Then the memstore scanners
+      if (memStoreScanners != null) {
+        scanners.addAll(memStoreScanners);
+      }
+      return scanners;
+    } catch (Throwable t) {
+      clearAndClose(memStoreScanners);
+      throw t instanceof IOException ? (IOException) t : new IOException(t);
     }
-    return scanners;
   }
 
   /**
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
index 67c01fa..725d8e6 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
@@ -236,9 +236,10 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner
 
     store.addChangedReaderObserver(this);
 
+    List<KeyValueScanner> scanners = null;
     try {
       // Pass columns to try to filter out unnecessary StoreFiles.
-      List<KeyValueScanner> scanners = selectScannersFrom(store,
+      scanners = selectScannersFrom(store,
         store.getScanners(cacheBlocks, scanUsePread, false, matcher, scan.getStartRow(),
           scan.includeStartRow(), scan.getStopRow(), scan.includeStopRow(), this.readPt));
 
@@ -258,6 +259,7 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner
       // Combine all seeked scanners with a heap
       resetKVHeap(scanners, comparator);
     } catch (IOException e) {
+      clearAndClose(scanners);
       // remove us from the HStore#changedReaderObservers here or we'll have no chance to
       // and might cause memory leak
       store.deleteChangedReaderObserver(this);
@@ -870,6 +872,9 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner
   }
 
   private static void clearAndClose(List<KeyValueScanner> scanners) {
+    if (scanners == null) {
+      return;
+    }
     for (KeyValueScanner s : scanners) {
       s.close();
     }