From 216aec03a6f658f64801921e7d1c8f8c7e95626b Mon Sep 17 00:00:00 2001
From: Jason Gerlowski <gerlowskija@apache.org>
Date: Thu, 13 Aug 2020 21:21:31 -0400
Subject: [PATCH] SOLR-14677: Always close DIH EntityProcessor/DataSource
 (#1741)

Prior to this commit, the wrapup logic at the end of
DocBuilder.execute() closed out a series of DIH objects, but did so
in a way that an exception closing any of them resulted in the remainder
staying open.  This is especially problematic since Writer.close()
throws exceptions that DIH uses to determine the success/failure of the
run.

In practice this caused network errors sending DIH data to other Solr
nodes to result in leaked JDBC connections.

This commit changes DocBuilder's termination logic to handle exceptions
more gracefully, ensuring that errors closing a DIHWriter (for example)
don't prevent the closure of entity-processor and DataSource objects.
---
 solr/CHANGES.txt                              |  2 ++
 .../solr/handler/dataimport/DataSource.java   |  3 +-
 .../solr/handler/dataimport/DocBuilder.java   | 30 ++++++++++++++-----
 .../handler/dataimport/EntityProcessor.java   |  3 +-
 4 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index a6e64e14438..9945ecb0229 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -182,6 +182,8 @@ Bug Fixes
 
 * SOLR-14751: Zookeeper Admin screen not working for old ZK versions (janhoy)
 
+* SOLR-14677: Improve DIH termination logic to close all DataSources, EntityProcessors (Jason Gerlowski)
+
 Other Changes
 ---------------------
 
diff --git a/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DataSource.java b/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DataSource.java
index e217ddd09c6..aeded279cbe 100644
--- a/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DataSource.java
+++ b/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DataSource.java
@@ -16,6 +16,7 @@
  */
 package org.apache.solr.handler.dataimport;
 
+import java.io.Closeable;
 import java.util.Properties;
 
 /**
@@ -35,7 +36,7 @@ import java.util.Properties;
  *
  * @since solr 1.3
  */
-public abstract class DataSource<T> {
+public abstract class DataSource<T> implements Closeable {
 
   /**
    * Initializes the DataSource with the <code>Context</code> and
diff --git a/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DocBuilder.java b/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DocBuilder.java
index 0f8dd6ed323..8115695d40a 100644
--- a/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DocBuilder.java
+++ b/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DocBuilder.java
@@ -18,6 +18,7 @@ package org.apache.solr.handler.dataimport;
 
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.common.util.IOUtils;
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.handler.dataimport.config.ConfigNameConstants;
 import org.apache.solr.handler.dataimport.config.DIHConfiguration;
@@ -272,24 +273,39 @@ public class DocBuilder {
     } catch(Exception e)
     {
       throw new RuntimeException(e);
-    } finally
-    {
-      if (writer != null) {
-        writer.close();
+    } finally {
+      // Cannot use IOUtils.closeQuietly since DIH relies on exceptions bubbling out of writer.close() to indicate
+      // success/failure of the run.
+      RuntimeException raisedDuringClose = null;
+      try {
+        if (writer != null) {
+          writer.close();
+        }
+      } catch (RuntimeException e) {
+        if (log.isWarnEnabled()) {
+          log.warn("Exception encountered while closing DIHWriter " + writer + "; temporarily suppressing to ensure other DocBuilder elements are closed", e); // logOk
+        }
+        raisedDuringClose = e;
       }
+
       if (epwList != null) {
         closeEntityProcessorWrappers(epwList);
       }
       if(reqParams.isDebug()) {
         reqParams.getDebugInfo().debugVerboseOutput = getDebugLogger().output;
       }
+
+      if (raisedDuringClose != null) {
+        throw raisedDuringClose;
+      }
     }
   }
   private void closeEntityProcessorWrappers(List<EntityProcessorWrapper> epwList) {
     for(EntityProcessorWrapper epw : epwList) {
-      epw.close();
-      if(epw.getDatasource()!=null) {
-        epw.getDatasource().close();
+      IOUtils.closeQuietly(epw);
+
+      if(epw.getDatasource() != null) {
+        IOUtils.closeQuietly(epw.getDatasource());
       }
       closeEntityProcessorWrappers(epw.getChildren());
     }
diff --git a/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/EntityProcessor.java b/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/EntityProcessor.java
index 8cfbed9287a..7ded623486e 100644
--- a/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/EntityProcessor.java
+++ b/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/EntityProcessor.java
@@ -16,6 +16,7 @@
  */
 package org.apache.solr.handler.dataimport;
 
+import java.io.Closeable;
 import java.util.Map;
 
 /**
@@ -36,7 +37,7 @@ import java.util.Map;
  *
  * @since solr 1.3
  */
-public abstract class EntityProcessor {
+public abstract class EntityProcessor implements Closeable {
 
   /**
    * This method is called when it starts processing an entity. When it comes