HADOOP-16665. Filesystems to be closed if they failed during initialize().

Contributed by Steve Loughran.

This FileSystem instantiation so if an IOException or RuntimeException is
raised in the invocation of FileSystem.initialize() then a best-effort
attempt is made to close the FS instance; exceptions raised that there
are swallowed.

The S3AFileSystem is also modified to do its own cleanup if an
IOException is raised during its initialize() process, it being the
FS we know has the "potential" to leak threads, especially in
extension points (e.g AWS Authenticators) which spawn threads.

Change-Id: Ib84073a606c9d53bf53cbfca4629876a03894f04

Author: steveloughran

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java

@@ -64,6 +64,7 @@
 import org.apache.hadoop.fs.permission.FsAction;
 import org.apache.hadoop.fs.permission.FsCreateModes;
 import org.apache.hadoop.fs.permission.FsPermission;
+import org.apache.hadoop.io.IOUtils;
 import org.apache.hadoop.io.MultipleIOException;
 import org.apache.hadoop.net.NetUtils;
 import org.apache.hadoop.security.AccessControlException;
@@ -3390,9 +3391,22 @@ private static FileSystem createFileSystem(URI uri, Configuration conf)
     Tracer tracer = FsTracer.get(conf);
     try(TraceScope scope = tracer.newScope("FileSystem#createFileSystem")) {
       scope.addKVAnnotation("scheme", uri.getScheme());
-      Class<?> clazz = getFileSystemClass(uri.getScheme(), conf);
-      FileSystem fs = (FileSystem)ReflectionUtils.newInstance(clazz, conf);
-      fs.initialize(uri, conf);
+      Class<? extends FileSystem> clazz =
+          getFileSystemClass(uri.getScheme(), conf);
+      FileSystem fs = ReflectionUtils.newInstance(clazz, conf);
+      try {
+        fs.initialize(uri, conf);
+      } catch (IOException | RuntimeException e) {
+        // exception raised during initialization.
+        // log summary at warn and full stack at debug
+        LOGGER.warn("Failed to initialize fileystem {}: {}",
+            uri, e.toString());
+        LOGGER.debug("Failed to initialize fileystem", e);
+        // then (robustly) close the FS, so as to invoke any
+        // cleanup code.
+        IOUtils.cleanupWithLogger(LOGGER, fs);
+        throw e;
+      }
       return fs;
     }
   }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemInitialization.java

@@ -18,14 +18,24 @@
 package org.apache.hadoop.fs;
 
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.permission.FsPermission;
+import org.apache.hadoop.util.Progressable;
 
+import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.net.URI;
 import java.net.URL;
 import java.util.ServiceConfigurationError;
 
 import org.junit.Test;
+
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.*;
 
+/**
+ * Tests related to filesystem creation and lifecycle.
+ */
 public class TestFileSystemInitialization {
 
  /**
@@ -55,4 +65,119 @@ public void testMissingLibraries() {
     } catch (Exception | ServiceConfigurationError expected) {
     }
   }
+
+  @Test
+  public void testNewInstanceFailure() throws Throwable {
+    intercept(IOException.class, FailingFileSystem.INITIALIZE, () ->
+        FileSystem.newInstance(new URI("failing://localhost"), FailingFileSystem
+            .failingConf()));
+    assertThat(FailingFileSystem.initCount).describedAs("init count")
+        .isEqualTo(1);
+    assertThat(FailingFileSystem.closeCount).describedAs("close count")
+        .isEqualTo(1);
+  }
+
+  /**
+   * An FS which will fail on both init and close, and update
+   * counters of invocations as it does so.
+   */
+  public static class FailingFileSystem extends FileSystem {
+
+    public static final String INITIALIZE = "initialize()";
+
+    public static final String CLOSE = "close()";
+
+    private static int initCount;
+
+    private static int closeCount;
+
+    private static Configuration failingConf() {
+      final Configuration conf = new Configuration(false);
+      conf.setClass("fs.failing.impl", FailingFileSystem.class,
+          FileSystem.class);
+      return conf;
+    }
+
+    @Override
+    public void initialize(final URI name, final Configuration conf)
+        throws IOException {
+      super.initialize(name, conf);
+      initCount++;
+      throw new IOException(INITIALIZE);
+    }
+
+    @Override
+    public void close() throws IOException {
+      closeCount++;
+      throw new IOException(CLOSE);
+    }
+
+    @Override
+    public URI getUri() {
+      return null;
+    }
+
+    @Override
+    public FSDataInputStream open(final Path f, final int bufferSize)
+        throws IOException {
+      return null;
+    }
+
+    @Override
+    public FSDataOutputStream create(final Path f,
+        final FsPermission permission,
+        final boolean overwrite,
+        final int bufferSize,
+        final short replication,
+        final long blockSize,
+        final Progressable progress) throws IOException {
+      return null;
+    }
+
+    @Override
+    public FSDataOutputStream append(final Path f,
+        final int bufferSize,
+        final Progressable progress) throws IOException {
+      return null;
+    }
+
+    @Override
+    public boolean rename(final Path src, final Path dst) throws IOException {
+      return false;
+    }
+
+    @Override
+    public boolean delete(final Path f, final boolean recursive)
+        throws IOException {
+      return false;
+    }
+
+    @Override
+    public FileStatus[] listStatus(final Path f)
+        throws FileNotFoundException, IOException {
+      return new FileStatus[0];
+    }
+
+    @Override
+    public void setWorkingDirectory(final Path new_dir) {
+
+    }
+
+    @Override
+    public Path getWorkingDirectory() {
+      return null;
+    }
+
+    @Override
+    public boolean mkdirs(final Path f, final FsPermission permission)
+        throws IOException {
+      return false;
+    }
+
+    @Override
+    public FileStatus getFileStatus(final Path f) throws IOException {
+      return null;
+    }
+  }
+
 }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/AbstractHadoopTestBase.java

@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.test;
+
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.rules.TestName;
+import org.junit.rules.Timeout;
+
+/**
+ * A base class for JUnit5+ tests that sets a default timeout for all tests
+ * that subclass this test.
+ *
+ * Threads are named to the method being executed, for ease of diagnostics
+ * in logs and thread dumps.
+ *
+ * Unlike {@link HadoopTestBase} this class does not extend JUnit Assert
+ * so is easier to use with AssertJ.
+ */
+public abstract class AbstractHadoopTestBase {
+
+  /**
+   * System property name to set the test timeout: {@value}.
+   */
+  public static final String PROPERTY_TEST_DEFAULT_TIMEOUT =
+      "test.default.timeout";
+
+  /**
+   * The default timeout (in milliseconds) if the system property
+   * {@link #PROPERTY_TEST_DEFAULT_TIMEOUT}
+   * is not set: {@value}.
+   */
+  public static final int TEST_DEFAULT_TIMEOUT_VALUE = 100000;
+
+  /**
+   * The JUnit rule that sets the default timeout for tests.
+   */
+  @Rule
+  public Timeout defaultTimeout = retrieveTestTimeout();
+
+  /**
+   * Retrieve the test timeout from the system property
+   * {@link #PROPERTY_TEST_DEFAULT_TIMEOUT}, falling back to
+   * the value in {@link #TEST_DEFAULT_TIMEOUT_VALUE} if the
+   * property is not defined.
+   * @return the recommended timeout for tests
+   */
+  public static Timeout retrieveTestTimeout() {
+    String propval = System.getProperty(PROPERTY_TEST_DEFAULT_TIMEOUT,
+                                         Integer.toString(
+                                           TEST_DEFAULT_TIMEOUT_VALUE));
+    int millis;
+    try {
+      millis = Integer.parseInt(propval);
+    } catch (NumberFormatException e) {
+      //fall back to the default value, as the property cannot be parsed
+      millis = TEST_DEFAULT_TIMEOUT_VALUE;
+    }
+    return new Timeout(millis, TimeUnit.MILLISECONDS);
+  }
+
+  /**
+   * The method name.
+   */
+  @Rule
+  public TestName methodName = new TestName();
+
+  /**
+   * Get the method name; defaults to the value of {@link #methodName}.
+   * Subclasses may wish to override it, which will tune the thread naming.
+   * @return the name of the method.
+   */
+  protected String getMethodName() {
+    return methodName.getMethodName();
+  }
+
+  /**
+   * Static initializer names this thread "JUnit".
+   */
+  @BeforeClass
+  public static void nameTestThread() {
+    Thread.currentThread().setName("JUnit");
+  }
+
+  /**
+   * Before each method, the thread is renamed to match the method name.
+   */
+  @Before
+  public void nameThreadToMethod() {
+    Thread.currentThread().setName("JUnit-" + getMethodName());
+  }
+}

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/HadoopTestBase.java

@@ -17,6 +17,8 @@
  */
 package org.apache.hadoop.test;
 
+import java.util.concurrent.TimeUnit;
+
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.BeforeClass;
@@ -70,7 +72,7 @@ public static Timeout retrieveTestTimeout() {
       //fall back to the default value, as the property cannot be parsed
       millis = TEST_DEFAULT_TIMEOUT_VALUE;
     }
-    return new Timeout(millis);
+    return new Timeout(millis, TimeUnit.MILLISECONDS);
   }
 
   /**

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ABlockOutputStream.java

@@ -53,6 +53,7 @@
 
 import static org.apache.hadoop.fs.s3a.S3AUtils.*;
 import static org.apache.hadoop.fs.s3a.Statistic.*;
+import static org.apache.hadoop.io.IOUtils.cleanupWithLogger;
 
 /**
  * Upload files/parts directly via different buffering mechanisms:
@@ -396,9 +397,9 @@ public void close() throws IOException {
       writeOperationHelper.writeFailed(ioe);
       throw ioe;
     } finally {
-      closeAll(LOG, block, blockFactory);
+      cleanupWithLogger(LOG, block, blockFactory);
       LOG.debug("Statistics: {}", statistics);
-      closeAll(LOG, statistics);
+      cleanupWithLogger(LOG, statistics);
       clearActiveBlock();
     }
     // Note end of write. This does not change the state of the remote FS.
@@ -437,7 +438,7 @@ private int putObject() throws IOException {
             // stream afterwards.
             return writeOperationHelper.putObject(putObjectRequest);
           } finally {
-            closeAll(LOG, uploadData, block);
+            cleanupWithLogger(LOG, uploadData, block);
           }
         });
     clearActiveBlock();
@@ -614,7 +615,7 @@ private void uploadBlockAsync(final S3ADataBlocks.DataBlock block)
               return partETag;
             } finally {
               // close the stream and block
-              closeAll(LOG, uploadData, block);
+              cleanupWithLogger(LOG, uploadData, block);
             }
           });
       partETagsFutures.add(partETagFuture);

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ADataBlocks.java

@@ -40,7 +40,7 @@
 import org.apache.hadoop.util.DirectBufferPool;
 
 import static org.apache.hadoop.fs.s3a.S3ADataBlocks.DataBlock.DestState.*;
-import static org.apache.hadoop.fs.s3a.S3AUtils.closeAll;
+import static org.apache.hadoop.io.IOUtils.cleanupWithLogger;
 
 /**
  * Set of classes to support output streaming into blocks which are then
@@ -155,7 +155,7 @@ InputStream getUploadStream() {
      */
     @Override
     public void close() throws IOException {
-      closeAll(LOG, uploadStream);
+      cleanupWithLogger(LOG, uploadStream);
     }
   }
 

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/ITestAssumedRoleCommitOperations.java

@@ -26,14 +26,14 @@
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.s3a.S3AFileSystem;
-import org.apache.hadoop.fs.s3a.S3AUtils;
 import org.apache.hadoop.fs.s3a.commit.ITestCommitOperations;
 
 import static org.apache.hadoop.fs.s3a.Constants.ASSUMED_ROLE_ARN;
 import static org.apache.hadoop.fs.s3a.S3ATestUtils.assume;
 import static org.apache.hadoop.fs.s3a.auth.RoleModel.*;
 import static org.apache.hadoop.fs.s3a.auth.RolePolicies.*;
 import static org.apache.hadoop.fs.s3a.auth.RoleTestUtils.*;
+import static org.apache.hadoop.io.IOUtils.cleanupWithLogger;
 
 /**
  * Verify that the commit operations work with a restricted set of operations.
@@ -84,7 +84,7 @@ public void setup() throws Exception {
 
   @Override
   public void teardown() throws Exception {
-    S3AUtils.closeAll(LOG, roleFS);
+    cleanupWithLogger(LOG, roleFS);
     // switches getFileSystem() back to the full FS.
     roleFS = null;
     super.teardown();