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 <[email protected]> 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 <[email protected]> Authored: Fri Sep 9 11:16:58 2016 -0700 Committer: Internal Jenkins <[email protected]> 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"); } }
