DRILL-7056: Drill fails with NPE when starting in distributed mode & 31010 port is used

closes #1656

2 files changed, 83 insertions, 39 deletions

diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
index 689b06bf7..cdde4aa3e 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
@@ -129,21 +129,6 @@ public class WebServer implements AutoCloseable {
 
   private File tmpJavaScriptDir;
 
-  public File getTmpJavaScriptDir() {
-    if (tmpJavaScriptDir == null) {
-      tmpJavaScriptDir = org.apache.drill.shaded.guava.com.google.common.io.Files.createTempDir();
-      tmpJavaScriptDir.deleteOnExit();
-      //Perform All auto generated files at this point
-      try {
-        generateOptionsDescriptionJSFile();
-        generateFunctionJS();
-      } catch (IOException e) {
-        logger.error("Unable to create temp dir for JavaScripts. {}", e);
-      }
-    }
-    return tmpJavaScriptDir;
-  }
-
   /**
    * Create Jetty based web server.
    *
@@ -246,8 +231,8 @@ public class WebServer implements AutoCloseable {
     //Add Local path resource (This will allow access to dynamically created files like JavaScript)
     final ServletHolder dynamicHolder = new ServletHolder("dynamic", DefaultServlet.class);
     //Skip if unable to get a temp directory (e.g. during Unit tests)
-    if (getTmpJavaScriptDir() != null) {
-      dynamicHolder.setInitParameter("resourceBase", getTmpJavaScriptDir().getAbsolutePath());
+    if (getOrCreateTmpJavaScriptDir() != null) {
+      dynamicHolder.setInitParameter("resourceBase", getOrCreateTmpJavaScriptDir().getAbsolutePath());
       dynamicHolder.setInitParameter("dirAllowed", "true");
       dynamicHolder.setInitParameter("pathInfoOnly", "true");
       servletContextHandler.addServlet(dynamicHolder, "/dynamic/*");
@@ -467,43 +452,61 @@ public class WebServer implements AutoCloseable {
     if (embeddedJetty != null) {
       embeddedJetty.stop();
     }
-    //Deleting temp directory
-    FileUtils.deleteDirectory(getTmpJavaScriptDir());
+    // Deleting temp directory
+    FileUtils.deleteQuietly(tmpJavaScriptDir);
   }
 
   /**
+   * Creates if not exists, and returns File for temporary Javascript directory
+   * @return File handle
+   */
+  public File getOrCreateTmpJavaScriptDir() {
+    if (tmpJavaScriptDir == null && this.drillbit.getContext() != null) {
+      tmpJavaScriptDir = org.apache.drill.shaded.guava.com.google.common.io.Files.createTempDir();
+      // Perform All auto generated files at this point
+      try {
+        generateOptionsDescriptionJSFile();
+        generateFunctionJS();
+      } catch (IOException e) {
+        logger.error("Unable to create temp dir for JavaScripts. {}", e);
+      }
+    }
+    return tmpJavaScriptDir;
+  }
+
+
+  /**
    * Generate Options Description JavaScript to serve http://drillhost/options ACE library search features
    * @throws IOException
    */
   private void generateOptionsDescriptionJSFile() throws IOException {
-    //Obtain list of Options & their descriptions
+    // Obtain list of Options & their descriptions
     OptionManager optionManager = this.drillbit.getContext().getOptionManager();
     OptionList publicOptions = optionManager.getPublicOptionList();
     List<OptionValue> options = new ArrayList<>(publicOptions);
-    //Add internal options
+    // Add internal options
     OptionList internalOptions = optionManager.getInternalOptionList();
     options.addAll(internalOptions);
     Collections.sort(options);
     int numLeftToWrite = options.size();
 
-    //Template source Javascript file
+    // Template source Javascript file
     InputStream optionsDescripTemplateStream = Resource.newClassPathResource(OPTIONS_DESCRIBE_TEMPLATE_JS).getInputStream();
-    //Generated file
-    File optionsDescriptionFile = new File(getTmpJavaScriptDir(), OPTIONS_DESCRIBE_JS);
+    // Generated file
+    File optionsDescriptionFile = new File(getOrCreateTmpJavaScriptDir(), OPTIONS_DESCRIBE_JS);
     final String file_content_footer = "};";
-    optionsDescriptionFile.deleteOnExit();
-    //Create a copy of a template and write with that!
+    // Create a copy of a template and write with that!
     java.nio.file.Files.copy(optionsDescripTemplateStream, optionsDescriptionFile.toPath());
     logger.info("Will write {} descriptions to {}", numLeftToWrite, optionsDescriptionFile.getAbsolutePath());
 
     try (BufferedWriter writer = new BufferedWriter(new FileWriter(optionsDescriptionFile, true))) {
-      //Iterate through options
+      // Iterate through options
       for (OptionValue option : options) {
         numLeftToWrite--;
         String optionName = option.getName();
         OptionDescription optionDescription = optionManager.getOptionDefinition(optionName).getValidator().getOptionDescription();
         if (optionDescription != null) {
-          //Note: We don't need to worry about short descriptions for WebUI, since they will never be explicitly accessed from the map
+          // Note: We don't need to worry about short descriptions for WebUI, since they will never be explicitly accessed from the map
           writer.append("  \"").append(optionName).append("\" : \"")
           .append(StringEscapeUtils.escapeEcmaScript(optionDescription.getDescription()))
           .append( numLeftToWrite > 0 ? "\"," : "\"");
@@ -521,14 +524,14 @@ public class WebServer implements AutoCloseable {
    * @throws IOException
    */
   private void generateFunctionJS() throws IOException {
-    //Naturally ordered set of function names
+    // Naturally ordered set of function names
     TreeSet<String> functionSet = new TreeSet<>();
-    //Extracting ONLY builtIn functions (i.e those already available)
+    // Extracting ONLY builtIn functions (i.e those already available)
     List<FunctionHolder> builtInFuncHolderList = this.drillbit.getContext().getFunctionImplementationRegistry().getLocalFunctionRegistry()
         .getAllJarsWithFunctionsHolders().get(LocalFunctionRegistry.BUILT_IN);
 
-    //Build List of 'usable' functions (i.e. functions that start with an alphabet and can be autocompleted by the ACE library)
-    //Example of 'unusable' functions would be operators like '<', '!'
+    // Build List of 'usable' functions (i.e. functions that start with an alphabet and can be autocompleted by the ACE library)
+    // Example of 'unusable' functions would be operators like '<', '!'
     int skipCount = 0;
     for (FunctionHolder builtInFunctionHolder : builtInFuncHolderList) {
       String name = builtInFunctionHolder.getName();
@@ -541,22 +544,21 @@ public class WebServer implements AutoCloseable {
     }
     logger.debug("{} functions will not be available in WebUI", skipCount);
 
-    //Generated file
-    File functionsListFile = new File(getTmpJavaScriptDir(), ACE_MODE_SQL_JS);
-    functionsListFile.deleteOnExit();
-    //Template source Javascript file
+    // Generated file
+    File functionsListFile = new File(getOrCreateTmpJavaScriptDir(), ACE_MODE_SQL_JS);
+    // Template source Javascript file
     try (InputStream aceModeSqlTemplateStream = Resource.newClassPathResource(ACE_MODE_SQL_TEMPLATE_JS).getInputStream()) {
-      //Create a copy of a template and write with that!
+      // Create a copy of a template and write with that!
       java.nio.file.Files.copy(aceModeSqlTemplateStream, functionsListFile.toPath());
     }
 
-    //Construct String
+    // Construct String
     String funcListString = String.join("|", functionSet);
 
     Path path = Paths.get(functionsListFile.getPath());
     try (Stream<String> lines = Files.lines(path)) {
       List <String> replaced =
-          lines //Replacing first occurrence
+          lines // Replacing first occurrence
             .map(line -> line.replaceFirst(DRILL_FUNCTIONS_PLACEHOLDER, funcListString))
             .collect(Collectors.toList());
       Files.write(path, replaced);
diff --git a/exec/java-exec/src/test/java/org/apache/drill/test/TestGracefulShutdown.java b/exec/java-exec/src/test/java/org/apache/drill/test/TestGracefulShutdown.java
index a433237c5..d74f1e691 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/test/TestGracefulShutdown.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/test/TestGracefulShutdown.java
@@ -17,11 +17,13 @@
  */
 package org.apache.drill.test;
 
+import org.apache.commons.lang3.reflect.FieldUtils;
 import org.apache.drill.categories.SlowTest;
 import org.apache.drill.common.exceptions.UserException;
 import org.apache.drill.exec.ExecConstants;
 import org.apache.drill.exec.proto.CoordinationProtos.DrillbitEndpoint;
 import org.apache.drill.exec.server.Drillbit;
+import org.apache.drill.exec.server.rest.WebServer;
 import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Rule;
@@ -30,17 +32,21 @@ import org.junit.experimental.categories.Category;
 import org.junit.rules.TestRule;
 
 import java.io.BufferedWriter;
+import java.io.File;
 import java.io.FileWriter;
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.lang.reflect.Field;
 import java.net.HttpURLConnection;
 import java.net.URL;
 import java.nio.file.Path;
 import java.util.Collection;
 
 import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 @Category({SlowTest.class})
@@ -205,6 +211,42 @@ public class TestGracefulShutdown extends BaseTestQuery {
     }
   }
 
+  @Test // DRILL-7056
+  public void testDrillbitTempDir() throws Exception {
+    File originalDrillbitTempDir = null;
+    ClusterFixtureBuilder fixtureBuilder = ClusterFixture.bareBuilder(dirTestWatcher).withLocalZk()
+        .configProperty(ExecConstants.ALLOW_LOOPBACK_ADDRESS_BINDING, true)
+        .configProperty(ExecConstants.INITIAL_USER_PORT, QueryTestUtil.getFreePortNumber(31170, 300))
+        .configProperty(ExecConstants.INITIAL_BIT_PORT, QueryTestUtil.getFreePortNumber(31180, 300));
+
+    try (ClusterFixture fixture = fixtureBuilder.build();
+        Drillbit twinDrillbitOnSamePort = new Drillbit(fixture.config(),
+            fixtureBuilder.configBuilder().getDefinitions(), fixture.serviceSet())) {
+      // Assert preconditions :
+      //      1. First drillbit instance should be started normally
+      //      2. Second instance startup should fail, because ports are occupied by the first one
+      Drillbit originalDrillbit = fixture.drillbit();
+      assertNotNull("First drillbit instance should be initialized", originalDrillbit);
+      originalDrillbitTempDir = getWebServerTempDirPath(originalDrillbit);
+      assertTrue("First drillbit instance should have a temporary Javascript dir initialized", originalDrillbitTempDir.exists());
+      try {
+        twinDrillbitOnSamePort.run();
+        fail("Invocation of 'twinDrillbitOnSamePort.run()' should throw UserException");
+      } catch (UserException userEx) {
+        assertThat(userEx.getMessage(), containsString("RESOURCE ERROR: Drillbit could not bind to port"));
+      }
+    }
+    // Verify deletion
+    assertFalse("First drillbit instance should have a temporary Javascript dir deleted", originalDrillbitTempDir.exists());
+  }
+
+  private static File getWebServerTempDirPath(Drillbit drillbit) throws IllegalAccessException {
+    Field webServerField = FieldUtils.getField(drillbit.getClass(), "webServer", true);
+    WebServer webServerHandle = (WebServer) FieldUtils.readField(webServerField, drillbit, true);
+    File webServerTempDirPath = webServerHandle.getOrCreateTmpJavaScriptDir();
+    return webServerTempDirPath;
+  }
+
   private static boolean waitAndAssertDrillbitCount(ClusterFixture cluster, int zkRefresh) throws InterruptedException {
 
     while (true) {