You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hive.apache.org by th...@apache.org on 2015/10/16 21:58:34 UTC
hive git commit: HIVE-11768 : java.io.DeleteOnExitHook leaks memory
on long running Hive Server2 Instances (Navis Ryu via Thejas Nair)
Repository: hive
Updated Branches:
refs/heads/master 3e7d1c45f -> bb05af06e
HIVE-11768 : java.io.DeleteOnExitHook leaks memory on long running Hive Server2 Instances (Navis Ryu via Thejas Nair)
Project: http://git-wip-us.apache.org/repos/asf/hive/repo
Commit: http://git-wip-us.apache.org/repos/asf/hive/commit/bb05af06
Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/bb05af06
Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/bb05af06
Branch: refs/heads/master
Commit: bb05af06e9049c23efc540e4a9b0f971068e3bd2
Parents: 3e7d1c4
Author: Navis Ryu <na...@apache.org>
Authored: Fri Oct 16 12:57:23 2015 -0700
Committer: Thejas Nair <th...@hortonworks.com>
Committed: Fri Oct 16 12:57:23 2015 -0700
----------------------------------------------------------------------
.../apache/hadoop/hive/common/FileUtils.java | 38 ++++++++++++++
.../hive/common/util/ShutdownHookManager.java | 52 +++++++++++++++++++-
.../common/util/TestShutdownHookManager.java | 22 +++++++--
.../hadoop/hive/ql/session/SessionState.java | 38 ++++++--------
.../cli/operation/HiveCommandOperation.java | 6 +--
.../service/cli/operation/SQLOperation.java | 9 +---
6 files changed, 123 insertions(+), 42 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/hive/blob/bb05af06/common/src/java/org/apache/hadoop/hive/common/FileUtils.java
----------------------------------------------------------------------
diff --git a/common/src/java/org/apache/hadoop/hive/common/FileUtils.java b/common/src/java/org/apache/hadoop/hive/common/FileUtils.java
index 7e4f386..d781f08 100644
--- a/common/src/java/org/apache/hadoop/hive/common/FileUtils.java
+++ b/common/src/java/org/apache/hadoop/hive/common/FileUtils.java
@@ -18,6 +18,7 @@
package org.apache.hadoop.hive.common;
+import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.net.URI;
@@ -44,6 +45,7 @@ import org.apache.hadoop.hive.shims.ShimLoader;
import org.apache.hadoop.hive.shims.Utils;
import org.apache.hadoop.security.UserGroupInformation;
import org.apache.hadoop.util.Shell;
+import org.apache.hive.common.util.ShutdownHookManager;
/**
@@ -766,4 +768,40 @@ public final class FileUtils {
return null;
}
}
+
+ public static void deleteDirectory(File directory) throws IOException {
+ org.apache.commons.io.FileUtils.deleteDirectory(directory);
+ }
+
+ /**
+ * create temporary file and register it to delete-on-exit hook.
+ * File.deleteOnExit is not used for possible memory leakage.
+ */
+ public static File createTempFile(String lScratchDir, String prefix, String suffix) throws IOException {
+ File tmpDir = lScratchDir == null ? null : new File(lScratchDir);
+ if (tmpDir != null && !tmpDir.exists() && !tmpDir.mkdirs()) {
+ // Do another exists to check to handle possible race condition
+ // Another thread might have created the dir, if that is why
+ // mkdirs returned false, that is fine
+ if (!tmpDir.exists()) {
+ throw new RuntimeException("Unable to create temp directory "
+ + lScratchDir);
+ }
+ }
+ File tmpFile = File.createTempFile(prefix, suffix, tmpDir);
+ ShutdownHookManager.deleteOnExit(tmpFile);
+ return tmpFile;
+ }
+
+ /**
+ * delete a temporary file and remove it from delete-on-exit hook.
+ */
+ public static boolean deleteTmpFile(File tempFile) {
+ if (tempFile != null) {
+ tempFile.delete();
+ ShutdownHookManager.cancelDeleteOnExit(tempFile);
+ return true;
+ }
+ return false;
+ }
}
http://git-wip-us.apache.org/repos/asf/hive/blob/bb05af06/common/src/java/org/apache/hive/common/util/ShutdownHookManager.java
----------------------------------------------------------------------
diff --git a/common/src/java/org/apache/hive/common/util/ShutdownHookManager.java b/common/src/java/org/apache/hive/common/util/ShutdownHookManager.java
index fd2f20a..0392eb5 100644
--- a/common/src/java/org/apache/hive/common/util/ShutdownHookManager.java
+++ b/common/src/java/org/apache/hive/common/util/ShutdownHookManager.java
@@ -18,6 +18,9 @@
package org.apache.hive.common.util;
+import com.google.common.annotations.VisibleForTesting;
+
+import java.io.File;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
@@ -44,15 +47,18 @@ public class ShutdownHookManager {
private static final ShutdownHookManager MGR = new ShutdownHookManager();
+ private static final DeleteOnExitHook DELETE_ON_EXIT_HOOK = new DeleteOnExitHook();
+
private static final Log LOG = LogFactory.getLog(ShutdownHookManager.class);
static {
+ MGR.addShutdownHookInternal(DELETE_ON_EXIT_HOOK, -1);
Runtime.getRuntime().addShutdownHook(
new Thread() {
@Override
public void run() {
MGR.shutdownInProgress.set(true);
- for (Runnable hook: MGR.getShutdownHooksInOrder()) {
+ for (Runnable hook : getShutdownHooksInOrder()) {
try {
hook.run();
} catch (Throwable ex) {
@@ -115,7 +121,7 @@ public class ShutdownHookManager {
return MGR.getShutdownHooksInOrderInternal();
}
- List<Runnable> getShutdownHooksInOrderInternal() {
+ private List<Runnable> getShutdownHooksInOrderInternal() {
List<HookEntry> list;
synchronized (MGR.hooks) {
list = new ArrayList<HookEntry>(MGR.hooks);
@@ -145,6 +151,9 @@ public class ShutdownHookManager {
* @param priority priority of the shutdownHook.
*/
public static void addShutdownHook(Runnable shutdownHook, int priority) {
+ if (priority < 0) {
+ throw new IllegalArgumentException("Priority should be greater than or equal to zero");
+ }
MGR.addShutdownHookInternal(shutdownHook, priority);
}
@@ -202,4 +211,43 @@ public class ShutdownHookManager {
private boolean isShutdownInProgressInternal() {
return shutdownInProgress.get();
}
+
+ /**
+ * register file to delete-on-exit hook
+ *
+ * @see {@link org.apache.hadoop.hive.common.FileUtils#createTempFile}
+ */
+ public static void deleteOnExit(File file) {
+ if (isShutdownInProgress()) {
+ throw new IllegalStateException("Shutdown in progress, cannot add a deleteOnExit");
+ }
+ DELETE_ON_EXIT_HOOK.deleteTargets.add(file);
+ }
+
+ /**
+ * deregister file from delete-on-exit hook
+ */
+ public static void cancelDeleteOnExit(File file) {
+ if (isShutdownInProgress()) {
+ throw new IllegalStateException("Shutdown in progress, cannot cancel a deleteOnExit");
+ }
+ DELETE_ON_EXIT_HOOK.deleteTargets.remove(file);
+ }
+
+ @VisibleForTesting
+ static boolean isRegisteredToDeleteOnExit(File file) {
+ return DELETE_ON_EXIT_HOOK.deleteTargets.contains(file);
+ }
+
+ private static class DeleteOnExitHook implements Runnable {
+ private final Set<File> deleteTargets = Collections.synchronizedSet(new HashSet<File>());
+
+ @Override
+ public void run() {
+ for (File deleteTarget : deleteTargets) {
+ deleteTarget.delete();
+ }
+ deleteTargets.clear();
+ }
+ }
}
http://git-wip-us.apache.org/repos/asf/hive/blob/bb05af06/common/src/test/org/apache/hive/common/util/TestShutdownHookManager.java
----------------------------------------------------------------------
diff --git a/common/src/test/org/apache/hive/common/util/TestShutdownHookManager.java b/common/src/test/org/apache/hive/common/util/TestShutdownHookManager.java
index fa30f15..66f6073 100644
--- a/common/src/test/org/apache/hive/common/util/TestShutdownHookManager.java
+++ b/common/src/test/org/apache/hive/common/util/TestShutdownHookManager.java
@@ -21,6 +21,11 @@ package org.apache.hive.common.util;
import org.junit.Assert;
import org.junit.Test;
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.hadoop.hive.common.FileUtils;
+
/**
* TestShutdownHookManager.
*
@@ -30,7 +35,7 @@ public class TestShutdownHookManager {
@Test
public void shutdownHookManager() {
- Assert.assertEquals(0, ShutdownHookManager.getShutdownHooksInOrder().size());
+ Assert.assertEquals(1, ShutdownHookManager.getShutdownHooksInOrder().size());
Runnable hook1 = new Runnable() {
@Override
public void run() {
@@ -44,23 +49,30 @@ public class TestShutdownHookManager {
ShutdownHookManager.addShutdownHook(hook1, 0);
Assert.assertTrue(ShutdownHookManager.hasShutdownHook(hook1));
- Assert.assertEquals(1, ShutdownHookManager.getShutdownHooksInOrder().size());
+ Assert.assertEquals(2, ShutdownHookManager.getShutdownHooksInOrder().size());
Assert.assertEquals(hook1, ShutdownHookManager.getShutdownHooksInOrder().get(0));
ShutdownHookManager.removeShutdownHook(hook1);
Assert.assertFalse(ShutdownHookManager.hasShutdownHook(hook1));
ShutdownHookManager.addShutdownHook(hook1, 0);
Assert.assertTrue(ShutdownHookManager.hasShutdownHook(hook1));
- Assert.assertEquals(1, ShutdownHookManager.getShutdownHooksInOrder().size());
+ Assert.assertEquals(2, ShutdownHookManager.getShutdownHooksInOrder().size());
Assert.assertTrue(ShutdownHookManager.hasShutdownHook(hook1));
- Assert.assertEquals(1, ShutdownHookManager.getShutdownHooksInOrder().size());
+ Assert.assertEquals(2, ShutdownHookManager.getShutdownHooksInOrder().size());
ShutdownHookManager.addShutdownHook(hook2, 1);
Assert.assertTrue(ShutdownHookManager.hasShutdownHook(hook1));
Assert.assertTrue(ShutdownHookManager.hasShutdownHook(hook2));
- Assert.assertEquals(2, ShutdownHookManager.getShutdownHooksInOrder().size());
+ Assert.assertEquals(3, ShutdownHookManager.getShutdownHooksInOrder().size());
Assert.assertEquals(hook2, ShutdownHookManager.getShutdownHooksInOrder().get(0));
Assert.assertEquals(hook1, ShutdownHookManager.getShutdownHooksInOrder().get(1));
+ }
+ @Test
+ public void deleteOnExit() throws IOException {
+ File file = FileUtils.createTempFile(null, "tmp", null);
+ Assert.assertTrue(ShutdownHookManager.isRegisteredToDeleteOnExit(file));
+ FileUtils.deleteTmpFile(file);
+ Assert.assertFalse(ShutdownHookManager.isRegisteredToDeleteOnExit(file));
}
}
http://git-wip-us.apache.org/repos/asf/hive/blob/bb05af06/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
----------------------------------------------------------------------
diff --git a/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java b/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
index 92ac209..34ec4d8 100644
--- a/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
+++ b/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
@@ -23,8 +23,6 @@ import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.PrintStream;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URLClassLoader;
@@ -43,7 +41,6 @@ import java.util.UUID;
import java.util.concurrent.CancellationException;
import java.util.concurrent.locks.ReentrantLock;
-import org.apache.commons.io.FileUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
@@ -52,6 +49,7 @@ import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.FileUtil;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.permission.FsPermission;
+import org.apache.hadoop.hive.common.FileUtils;
import org.apache.hadoop.hive.common.JavaUtils;
import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
@@ -300,6 +298,14 @@ public class SessionState {
this.tmpErrOutputFile = tmpErrOutputFile;
}
+ public void deleteTmpOutputFile() {
+ FileUtils.deleteTmpFile(tmpOutputFile);
+ }
+
+ public void deleteTmpErrOutputFile() {
+ FileUtils.deleteTmpFile(tmpErrOutputFile);
+ }
+
public boolean getIsSilent() {
if(conf != null) {
return conf.getBoolVar(HiveConf.ConfVars.HIVESESSIONSILENT);
@@ -725,9 +731,8 @@ public class SessionState {
if (localSessionPath != null) {
FileSystem.getLocal(conf).delete(localSessionPath, true);
}
- if (this.getTmpOutputFile().exists()) {
- this.getTmpOutputFile().delete();
- }
+ deleteTmpOutputFile();
+ deleteTmpErrOutputFile();
}
/**
@@ -835,25 +840,10 @@ public class SessionState {
* @throws IOException
*/
private static File createTempFile(HiveConf conf) throws IOException {
- String lScratchDir =
- HiveConf.getVar(conf, HiveConf.ConfVars.LOCALSCRATCHDIR);
-
- File tmpDir = new File(lScratchDir);
+ String lScratchDir = HiveConf.getVar(conf, HiveConf.ConfVars.LOCALSCRATCHDIR);
String sessionID = conf.getVar(HiveConf.ConfVars.HIVESESSIONID);
- if (!tmpDir.exists()) {
- if (!tmpDir.mkdirs()) {
- //Do another exists to check to handle possible race condition
- // Another thread might have created the dir, if that is why
- // mkdirs returned false, that is fine
- if(!tmpDir.exists()){
- throw new RuntimeException("Unable to create log directory "
- + lScratchDir);
- }
- }
- }
- File tmpFile = File.createTempFile(sessionID, ".pipeout", tmpDir);
- tmpFile.deleteOnExit();
- return tmpFile;
+
+ return FileUtils.createTempFile(lScratchDir, sessionID, ".pipeout");
}
/**
http://git-wip-us.apache.org/repos/asf/hive/blob/bb05af06/service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java
----------------------------------------------------------------------
diff --git a/service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java b/service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java
index 1d1e995..c40c269 100644
--- a/service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java
+++ b/service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java
@@ -200,10 +200,8 @@ public class HiveCommandOperation extends ExecuteStatementOperation {
private void cleanTmpFile() {
resetResultReader();
SessionState sessionState = getParentSession().getSessionState();
- File tmp = sessionState.getTmpOutputFile();
- tmp.delete();
- tmp = sessionState.getTmpErrOutputFile();
- tmp.delete();
+ sessionState.deleteTmpOutputFile();
+ sessionState.deleteTmpErrOutputFile();
}
private void resetResultReader() {
http://git-wip-us.apache.org/repos/asf/hive/blob/bb05af06/service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
----------------------------------------------------------------------
diff --git a/service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java b/service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
index 175348b..e2ee388 100644
--- a/service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
+++ b/service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
@@ -293,13 +293,8 @@ public class SQLOperation extends ExecuteStatementOperation {
driver = null;
SessionState ss = SessionState.get();
- if (ss.getTmpOutputFile() != null) {
- ss.getTmpOutputFile().delete();
- }
-
- if (ss.getTmpErrOutputFile() != null) {
- ss.getTmpErrOutputFile().delete();
- }
+ ss.deleteTmpOutputFile();
+ ss.deleteTmpErrOutputFile();
}
@Override