Make exception_wrapper::throwException() strict

Summary:
The definition of `throwException()` is changed to add the `[[noreturn]]`
attribute for consistency with `std::rethrow_exception`. If the
`exception_wrapper` contains an exception, that is rethrown; if not, then
`std::terminate` is called.

An alternative design which threw an exception if the `exception_wrapper` did not contain an exception was rejected because of the ambiguity it introduced as to whether the exception thrown was wrapped or not.

Benchmarks before:

```

============================================================================
folly/test/ExceptionWrapperBenchmark.cpp        relative  time/iter  iters/s
============================================================================
exception_ptr_create_and_test                                1.45us  689.01K
exception_wrapper_create_and_test               4337.80%    33.46ns   29.89M
----------------------------------------------------------------------------
exception_ptr_create_and_test_concurrent                   342.99us    2.92K
exception_wrapper_create_and_test_concurrent     101.41%   338.21us    2.96K
----------------------------------------------------------------------------
exception_ptr_create_and_throw                               3.05us  327.89K
exception_wrapper_create_and_throw               140.22%     2.17us  459.77K
exception_wrapper_create_and_cast               8956.80%    34.05ns   29.37M
----------------------------------------------------------------------------
exception_ptr_create_and_throw_concurrent                  372.68us    2.68K
exception_wrapper_create_and_throw_concurrent    102.54%   363.44us    2.75K
exception_wrapper_create_and_cast_concurrent     110.93%   335.97us    2.98K
============================================================================
```

and after:

```

============================================================================
folly/test/ExceptionWrapperBenchmark.cpp        relative  time/iter  iters/s
============================================================================
exception_ptr_create_and_test                                1.46us  684.09K
exception_wrapper_create_and_test               4368.84%    33.46ns   29.89M
----------------------------------------------------------------------------
exception_ptr_create_and_test_concurrent                   341.20us    2.93K
exception_wrapper_create_and_test_concurrent      99.88%   341.60us    2.93K
----------------------------------------------------------------------------
exception_ptr_create_and_throw                               3.08us  324.93K
exception_wrapper_create_and_throw               106.93%     2.88us  347.46K
exception_wrapper_create_and_cast               9532.05%    32.29ns   30.97M
----------------------------------------------------------------------------
exception_ptr_create_and_throw_concurrent                  363.73us    2.75K
exception_wrapper_create_and_throw_concurrent    101.04%   360.00us    2.78K
exception_wrapper_create_and_cast_concurrent     104.50%   348.07us    2.87K
============================================================================
```

Reviewed By: yfeldblum

Differential Revision: D3923939

fbshipit-source-id: 6fc58ac571e59e4122f1fbd194c678e3a3841057

diff --git a/folly/ExceptionWrapper.h b/folly/ExceptionWrapper.h
index ca57282c524ea0f293216bf8bc9acb416ec25696..94f6e82825fb927ba8eb9ffb70eb7035c80009d3 100644 (file)
--- a/folly/ExceptionWrapper.h
+++ b/folly/ExceptionWrapper.h
@@ -148,12 +148,15 @@ class exception_wrapper {
     assign_eptr(eptr);
   }
 
-  void throwException() const {
+  // If the exception_wrapper does not contain an exception, std::terminate()
+  // is invoked to assure the [[noreturn]] behaviour.
+  [[noreturn]] void throwException() const {
     if (throwfn_) {
       throwfn_(item_.get());
     } else if (eptr_) {
       std::rethrow_exception(eptr_);
     }
+    std::terminate();
   }
 
   explicit operator bool() const {
@@ -260,7 +263,9 @@ class exception_wrapper {
     bool>::type
   with_exception(F f) const {
     try {
-      throwException();
+      if (*this) {
+        throwException();
+      }
     } catch (typename std::decay<Ex>::type& e) {
       f(e);
       return true;
@@ -276,7 +281,9 @@ class exception_wrapper {
     }
 
     try {
-      throwException();
+      if (*this) {
+        throwException();
+      }
     } catch (...) {
       return std::current_exception();
     }

diff --git a/folly/test/ExceptionWrapperTest.cpp b/folly/test/ExceptionWrapperTest.cpp
index dc3a0ac69eabe9d5397a0e1efa450bcdb2783b12..954e92cec4b75ba2fb5e5cd1f53592bc2bd2ac63 100644 (file)
--- a/folly/test/ExceptionWrapperTest.cpp
+++ b/folly/test/ExceptionWrapperTest.cpp
@@ -170,9 +170,8 @@ TEST(ExceptionWrapper, with_exception_test) {
   EXPECT_TRUE(bool(ew));
   EXPECT_EQ(ew.what(), kIntExceptionClassName + ": int == 23");
   EXPECT_EQ(ew.class_name(), kIntExceptionClassName);
-  ew.with_exception([&](const IntException& ie) {
-      EXPECT_EQ(ie.getInt(), expected);
-    });
+  EXPECT_TRUE(ew.with_exception(
+      [&](const IntException& ie) { EXPECT_EQ(ie.getInt(), expected); }));
 
   // I can try_and_catch a non-copyable base class.  This will use
   // std::exception_ptr internally.
@@ -183,15 +182,21 @@ TEST(ExceptionWrapper, with_exception_test) {
   EXPECT_TRUE(bool(ew2));
   EXPECT_EQ(ew2.what(), kIntExceptionClassName + ": int == 23");
   EXPECT_EQ(ew2.class_name(), kIntExceptionClassName);
-  ew2.with_exception([&](AbstractIntException& ie) {
-      EXPECT_EQ(ie.getInt(), expected);
-      EXPECT_TRUE(dynamic_cast<IntException*>(&ie));
-    });
+  EXPECT_TRUE(ew2.with_exception([&](AbstractIntException& ie) {
+    EXPECT_EQ(ie.getInt(), expected);
+    EXPECT_TRUE(dynamic_cast<IntException*>(&ie));
+  }));
 
   // Test with const this.  If this compiles and does not crash due to
   // infinite loop when it runs, it succeeds.
   const exception_wrapper& cew = ew;
-  cew.with_exception([&](const IntException& /* ie */) { SUCCEED(); });
+  EXPECT_TRUE(
+      cew.with_exception([&](const IntException& /* ie */) { SUCCEED(); }));
+
+  // Test with empty ew.
+  exception_wrapper empty_ew;
+  EXPECT_FALSE(
+      empty_ew.with_exception([&](const std::exception& /* ie */) { FAIL(); }));
 
   // This won't even compile.  You can't use a function which takes a
   // non-const reference with a const exception_wrapper.
@@ -202,6 +207,33 @@ TEST(ExceptionWrapper, with_exception_test) {
 */
 }
 
+TEST(ExceptionWrapper, getExceptionPtr_test) {
+  int expected = 23;
+
+  // This works, and doesn't slice.
+  exception_wrapper ew = try_and_catch<std::exception, std::runtime_error>(
+      [=]() { throw IntException(expected); });
+  std::exception_ptr eptr = ew.getExceptionPtr();
+  EXPECT_THROW(std::rethrow_exception(eptr), IntException);
+
+  // I can try_and_catch a non-copyable base class.  This will use
+  // std::exception_ptr internally.
+  exception_wrapper ew2 = try_and_catch<AbstractIntException>(
+      [=]() { throw IntException(expected); });
+  eptr = ew2.getExceptionPtr();
+  EXPECT_THROW(std::rethrow_exception(eptr), IntException);
+
+  // Test with const this.
+  const exception_wrapper& cew = ew;
+  eptr = cew.getExceptionPtr();
+  EXPECT_THROW(std::rethrow_exception(eptr), IntException);
+
+  // Test with empty ew.
+  exception_wrapper empty_ew;
+  eptr = empty_ew.getExceptionPtr();
+  EXPECT_DEATH(std::rethrow_exception(eptr), "exception");
+}
+
 TEST(ExceptionWrapper, with_exception_deduction) {
   auto ew = make_exception_wrapper<std::runtime_error>("hi");
   EXPECT_TRUE(ew.with_exception([](std::runtime_error&) {}));
@@ -268,6 +300,11 @@ TEST(ExceptionWrapper, exceptionStr) {
   EXPECT_EQ(kRuntimeErrorClassName + ": argh", exceptionStr(ew));
 }
 
+TEST(ExceptionWrapper, throwException_noException) {
+  exception_wrapper ew;
+  ASSERT_DEATH(ew.throwException(), "exception");
+}
+
 namespace {
 class TestException : public std::exception { };
 void testEW(const exception_wrapper& ew) {