Mailing-List: contact commits-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: tarmstrong@apache.org
To: commits@impala.incubator.apache.org
Message-Id: <8968e652853e4a2fa98c27222df5a0f4@git.apache.org>
Subject: incubator-impala git commit: IMPALA-4111: backend death tests should
 not produce minidumps
Date: Fri, 16 Sep 2016 16:04:15 +0000 (UTC)
archived-at: Fri, 16 Sep 2016 16:04:22 -0000

Repository: incubator-impala
Updated Branches:
  refs/heads/master 3904d3091 -> e3abf84cc


IMPALA-4111: backend death tests should not produce minidumps

Move the existing core dump disabler into a shared utility header and
also disable minidumps too. Add a macro to simplify use of the disabler.

Change-Id: I33037f4f77c8188fc2ec46a77083a0a64f6ea404
Reviewed-on: http://gerrit.cloudera.org:8080/4353
Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com>
Tested-by: Internal Jenkins


Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/e3abf84c
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/e3abf84c
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/e3abf84c

Branch: refs/heads/master
Commit: e3abf84cce0dcc5fc5817688b9500b652550019d
Parents: 3904d30
Author: Tim Armstrong <tarmstrong@cloudera.com>
Authored: Fri Sep 9 11:16:58 2016 -0700
Committer: Internal Jenkins <cloudera-hudson@gerrit.cloudera.org>
Committed: Fri Sep 16 07:37:36 2016 +0000

----------------------------------------------------------------------
 be/src/testutil/death-test-util.h | 59 ++++++++++++++++++++++++++++++++++
 be/src/util/minidump.cc           | 19 +++++++++--
 be/src/util/minidump.h            |  4 +++
 be/src/util/promise-test.cc       | 25 ++------------
 4 files changed, 83 insertions(+), 24 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e3abf84c/be/src/testutil/death-test-util.h
----------------------------------------------------------------------
diff --git a/be/src/testutil/death-test-util.h b/be/src/testutil/death-test-util.h
new file mode 100644
index 0000000..d84b374
--- /dev/null
+++ b/be/src/testutil/death-test-util.h
@@ -0,0 +1,59 @@
+// 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.
+
+#ifndef IMPALA_TESTUTIL_DEATH_TEST_UTIL_H_
+#define IMPALA_TESTUTIL_DEATH_TEST_UTIL_H_
+
+#include <sys/resource.h>
+
+#include "util/minidump.h"
+
+// Wrapper around gtest's ASSERT_DEBUG_DEATH that prevents coredumps and minidumps
+// being generated as the result of the death test.
+#define IMPALA_ASSERT_DEBUG_DEATH(fn, msg)    \
+  do {                                        \
+    ScopedCoredumpDisabler disable_coredumps; \
+    ASSERT_DEBUG_DEATH(fn, msg);              \
+  } while (false);
+
+namespace impala {
+
+/// Utility class that disables coredumps and minidumps within a given scope.
+struct ScopedCoredumpDisabler {
+ public:
+  ScopedCoredumpDisabler() {
+    getrlimit(RLIMIT_CORE, &limit_before_);
+    rlimit limit;
+    limit.rlim_cur = limit.rlim_max = 0;
+    setrlimit(RLIMIT_CORE, &limit);
+
+    minidumps_enabled_before_ = EnableMinidumpsForTest(false);
+  }
+
+  ~ScopedCoredumpDisabler() {
+    setrlimit(RLIMIT_CORE, &limit_before_);
+
+    EnableMinidumpsForTest(minidumps_enabled_before_);
+  }
+
+ private:
+  rlimit limit_before_;
+  bool minidumps_enabled_before_;
+};
+}
+
+#endif

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e3abf84c/be/src/util/minidump.cc
----------------------------------------------------------------------
diff --git a/be/src/util/minidump.cc b/be/src/util/minidump.cc
index 23c7564..4567c48 100644
--- a/be/src/util/minidump.cc
+++ b/be/src/util/minidump.cc
@@ -56,6 +56,15 @@ namespace impala {
 /// files during process crashes, but also can be used to write minidumps directly.
 static google_breakpad::ExceptionHandler* minidump_exception_handler = NULL;
 
+/// Test helper. True if minidumps should be enabled.
+static bool minidumps_enabled = true;
+
+/// Called by the exception handler before minidump is produced. Minidump is only written
+/// if this returns true.
+static bool FilterCallback(void* context) {
+  return minidumps_enabled;
+}
+
 /// Callback for breakpad. It is called by breakpad whenever a minidump file has been
 /// written and should not be called directly. It logs the event before breakpad crashes
 /// the process. Due to the process being in a failed state we write to stdout/stderr and
@@ -225,8 +234,8 @@ Status RegisterMinidump(const char* cmd_line_path) {
 
   // Intentionally leaked. We want this to have the lifetime of the process.
   DCHECK(minidump_exception_handler == NULL);
-  minidump_exception_handler =
-      new google_breakpad::ExceptionHandler(desc, NULL, DumpCallback, NULL, true, -1);
+  minidump_exception_handler = new google_breakpad::ExceptionHandler(
+      desc, FilterCallback, DumpCallback, NULL, true, -1);
 
   // Setup signal handler for SIGUSR1.
   SetupSignalHandler();
@@ -234,4 +243,10 @@ Status RegisterMinidump(const char* cmd_line_path) {
   return Status::OK();
 }
 
+bool EnableMinidumpsForTest(bool enabled) {
+  bool old_value = minidumps_enabled;
+  minidumps_enabled = enabled;
+  return old_value;
+}
+
 }  // end ns impala

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e3abf84c/be/src/util/minidump.h
----------------------------------------------------------------------
diff --git a/be/src/util/minidump.h b/be/src/util/minidump.h
index 7750eaf..b2c7160 100644
--- a/be/src/util/minidump.h
+++ b/be/src/util/minidump.h
@@ -26,6 +26,10 @@ namespace impala {
 /// See https://chromium.googlesource.com/breakpad/breakpad/ for more details.
 Status RegisterMinidump(const char* cmd_line_path);
 
+/// Test helper to temporarily enable or disable minidumps, for example during death
+/// tests that deliberately trigger DCHECKs. Returns true if minidumps were previously
+/// enabled or false otherwise.
+bool EnableMinidumpsForTest(bool enabled);
 }
 
 #endif

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e3abf84c/be/src/util/promise-test.cc
----------------------------------------------------------------------
diff --git a/be/src/util/promise-test.cc b/be/src/util/promise-test.cc
index 1ce77c0..2906c27 100644
--- a/be/src/util/promise-test.cc
+++ b/be/src/util/promise-test.cc
@@ -16,10 +16,10 @@
 // under the License.
 
 #include <boost/thread.hpp>
-#include <sys/resource.h>
 
 #include "runtime/timestamp-value.h"
 #include "testutil/gtest-util.h"
+#include "testutil/death-test-util.h"
 #include "util/promise.h"
 #include "util/time.h"
 
@@ -27,23 +27,6 @@
 
 namespace impala {
 
-struct ScopedLimitResetter {
- public:
-  ScopedLimitResetter() {
-    getrlimit(RLIMIT_CORE, &limit_before_);
-    rlimit limit;
-    limit.rlim_cur = limit.rlim_max = 0;
-    setrlimit(RLIMIT_CORE, &limit);
-  }
-
-  ~ScopedLimitResetter() {
-    setrlimit(RLIMIT_CORE, &limit_before_);
-  }
-
- private:
-  rlimit limit_before_;
-};
-
 void RunThread(Promise<int64_t>* promise) {
   promise->Set(100);
 }
@@ -75,16 +58,14 @@ TEST(PromiseTest, TimeoutTest) {
 }
 
 TEST(PromiseDeathTest, RepeatedSetTest) {
-  // This test intentionally causes a crash. Don't generate core files for it.
-  ScopedLimitResetter resetter;
-
   // Hint to gtest that only one thread is being used here. Multiple threads are unsafe
   // for 'death' tests, see
   // https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests for more detail
   ::testing::FLAGS_gtest_death_test_style = "threadsafe";
   Promise<int64_t> promise;
   promise.Set(100);
-  ASSERT_DEBUG_DEATH(promise.Set(150), "Called Set\\(\\.\\.\\) twice on the same Promise");
+  IMPALA_ASSERT_DEBUG_DEATH(
+      promise.Set(150), "Called Set\\(\\.\\.\\) twice on the same Promise");
 }
 
 }