From 4f15ebe954994ef0afbf812de1564fce09cf5ff7 Mon Sep 17 00:00:00 2001 From: Trond Norbye Date: Fri, 1 Mar 2024 07:16:40 +0100 Subject: [PATCH] MB-44253: Remove try/catch from Executor*::runTask methods Slightly adapted Dave Rigby's patch: commit 3d4519d9771d55a05c65830cfd0c95cc1994d211 Author: Dave Rigby Date: Thu Feb 11 14:01:47 2021 +0000 MB-44253: Remove try/catch from Executor*::runTask methods These try/catch blocks prevent unhandled exceptions in GlobalTasks from triggering std::terminate, and in turn generating a Breakpad minidump with the state of the process when the exception was thrown (as opposed to where it was caught). Started a discussion with Folly about a more generic way to address this (i.e. making the try/catch somehow optional) - see https://github.com/facebook/folly/issues/1525 - but in the short-term simply remove the invokeCatchingExns() wrapper from our branch. (Note: Currently we only use ThreadPoolExecutor and hence we only /need/ to change that one, but for consistency / possible future use change all instances). Additionally disable the two unit tests which assert the removed exception-catching behaviour (they would now std::terminate): - ThreadedExecutorTest.exception - SerialExecutorTest/*.ExecutionThrows --- folly/Executor.h | 4 ++++ folly/executors/test/SerialExecutorTest.cpp | 5 ++++- folly/executors/test/ThreadedExecutorTest.cpp | 5 ++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/folly/Executor.h b/folly/Executor.h index fc8e9321fc1..c5104b370fa 100644 --- a/folly/Executor.h +++ b/folly/Executor.h @@ -341,7 +341,11 @@ class Executor { template FOLLY_ERASE static void invokeCatchingExns(char const* p, F f) noexcept { +#if 0 catch_exception(f, invokeCatchingExnsLog, p); +#else + f(); +#endif } protected: diff --git a/folly/executors/test/SerialExecutorTest.cpp b/folly/executors/test/SerialExecutorTest.cpp index 121fa80c2b7..7a1e04a3819 100644 --- a/folly/executors/test/SerialExecutorTest.cpp +++ b/folly/executors/test/SerialExecutorTest.cpp @@ -192,7 +192,10 @@ TYPED_TEST(SerialExecutorTest, RecursiveAddInline) { recursiveAddTest(folly::InlineExecutor::instance()); } -TYPED_TEST(SerialExecutorTest, ExecutionThrows) { +// MB-44253: Disabled - unhandled exceptions in tasks are no longer caught +// (invokeCatchingExns is a no-op on this branch), so this test would +// std::terminate. +TYPED_TEST(SerialExecutorTest, DISABLED_ExecutionThrows) { auto executor = TypeParam::create(); // an empty Func will throw std::bad_function_call when invoked, diff --git a/folly/executors/test/ThreadedExecutorTest.cpp b/folly/executors/test/ThreadedExecutorTest.cpp index 5aefa9b4e4e..4aa69827a13 100644 --- a/folly/executors/test/ThreadedExecutorTest.cpp +++ b/folly/executors/test/ThreadedExecutorTest.cpp @@ -39,7 +39,10 @@ TEST_F(ThreadedExecutorTest, example) { EXPECT_EQ("42", ret); } -TEST_F(ThreadedExecutorTest, exception) { +// MB-44253: Disabled - unhandled exceptions in tasks are no longer caught +// (invokeCatchingExns is a no-op on this branch), so this test would +// std::terminate. +TEST_F(ThreadedExecutorTest, DISABLED_exception) { folly::ThreadedExecutor x; x.add([] { throw std::runtime_error("This should not crash the program"); }); }