From: Marc Horowitz Date: Mon, 11 Aug 2014 22:40:23 +0000 (-0700) Subject: Make it work more generically X-Git-Tag: v0.22.0~381 X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=924520acb44b5c1349c229ad1b1d3457605e83d9;p=folly.git Make it work more generically Summary: Change the strategy here: provide a more generic interface which can be used safely with any exception types, but operates more efficiently for explicitly named types. Update the callers to use the new pattern, which mostly means get() and operator* are replaced with other more generic methods. (This diff was constructed by merging D1490304, D1501644, and D1500861.) Test Plan: run tests. Run something which generates an exception message, and make sure that message isn't "std::exception" Reviewed By: marccelani@fb.com Subscribers: ruibalp, mcduff, marccelani, hitesh, mshneer, rtgw-diffs@, alandau, bmatheny, adityab, wormhole-diffs@, bwester, njormrod FB internal diff: D1517701 --- diff --git a/folly/ExceptionWrapper.h b/folly/ExceptionWrapper.h index 726a27fc..14f2b71b 100644 --- a/folly/ExceptionWrapper.h +++ b/folly/ExceptionWrapper.h @@ -20,6 +20,7 @@ #include #include #include +#include #include namespace folly { @@ -50,11 +51,13 @@ namespace folly { * exception_wrapper is designed to handle exception management for both * convenience and high performance use cases. make_exception_wrapper is * templated on derived type, allowing us to rethrow the exception properly for - * users that prefer convenience. exception_wrapper is flexible enough to accept - * any std::exception. For performance sensitive applications, exception_wrapper - * exposes a get() function. These users can use dynamic_cast to retrieve - * desired derived types (hence the decision to limit usage to just - * std::exception instead of void*). + * users that prefer convenience. These explicitly named exception types can + * therefore be handled without any peformance penalty. exception_wrapper is + * also flexible enough to accept any type. If a caught exception is not of an + * explicitly named type, then std::exception_ptr is used to preserve the + * exception state. For performance sensitive applications, the accessor methods + * can test or extract a pointer to a specific exception type with very little + * overhead. * * Example usage: * @@ -86,16 +89,14 @@ namespace folly { * // Thread2: Exceptions are bad! * void processResult() { * auto ep = globalExceptionWrapper.get(); - * if (ep) { - * auto faceplant = dynamic_cast(ep); - * if (faceplant) { + * if (!ep.with_exception([&]( + * FacePlantException& faceplant) { * LOG(ERROR) << "FACEPLANT"; - * } else { - * auto failwhale = dynamic_cast(ep); - * if (failwhale) { + * })) { + * ep.with_exception([&]( + * FailWhaleException& failwhale) { * LOG(ERROR) << "FAILWHALE!"; - * } - * } + * }); * } * } * @@ -107,21 +108,80 @@ class exception_wrapper { void throwException() const { if (throwfn_) { throwfn_(item_.get()); + } else if (eptr_) { + std::rethrow_exception(eptr_); } } - std::exception* get() { return item_.get(); } - const std::exception* get() const { return item_.get(); } + explicit operator bool() const { + return item_ || eptr_; + } - std::exception* operator->() { return get(); } - const std::exception* operator->() const { return get(); } + // This will return a non-nullptr only if the exception is held as a + // copy. It is the only interface which will distinguish between an + // exception held this way, and by exception_ptr. You probably + // shouldn't use it at all. + std::exception* getCopied() { return item_.get(); } + const std::exception* getCopied() const { return item_.get(); } - std::exception& operator*() { assert(get()); return *get(); } - const std::exception& operator*() const { assert(get()); return *get(); } + fbstring what() const { + if (item_) { + return exceptionStr(*item_.get()); + } else if (eptr_) { + return estr_; + } else { + return fbstring(); + } + } - explicit operator bool() const { return get(); } + template + bool is_compatible_with() const { + if (item_) { + return dynamic_cast(getCopied()); + } else if (eptr_) { + try { + std::rethrow_exception(eptr_); + } catch (std::exception& e) { + return dynamic_cast(&e); + } catch (...) { + // fall through + } + } + return false; + } + + template + bool with_exception(F f) { + if (item_) { + if (auto ex = dynamic_cast(getCopied())) { + f(*ex); + return true; + } + } else if (eptr_) { + try { + std::rethrow_exception(eptr_); + } catch (std::exception& e) { + if (auto ex = dynamic_cast(&e)) { + f(*ex); + return true; + } + } catch (...) { + // fall through + } + } + return false; + } + + template + bool with_exception(F f) const { + return with_exception(f); + } std::exception_ptr getExceptionPtr() const { + if (eptr_) { + return eptr_; + } + try { throwException(); } catch (...) { @@ -131,8 +191,16 @@ class exception_wrapper { } protected: + // Optimized case: if we know what type the exception is, we can + // store a copy of the concrete type, and a helper function so we + // can rethrow it. std::shared_ptr item_; void (*throwfn_)(std::exception*); + // Fallback case: store the library wrapper, which is less efficient + // but gets the job done. Also store the the what() string, so we + // can at least get it back out without having to rethrow. + std::exception_ptr eptr_; + std::string estr_; template friend exception_wrapper make_exception_wrapper(Args&&... args); @@ -203,13 +271,52 @@ class try_and_catch : try_and_catch() : Base() {} + template + typename std::enable_if::value>::type + assign_eptr(Ex& e) { + this->eptr_ = std::current_exception(); + // The cast is needed so we get the desired overload of exceptionStr() + this->estr_ = exceptionStr(static_cast(e)).toStdString(); + } + + template + typename std::enable_if::value>::type + assign_eptr(Ex& e) { + this->eptr_ = std::current_exception(); + this->estr_ = exceptionStr(e).toStdString(); + } + + template + struct optimize { + static const bool value = + std::is_base_of::value && + std::is_copy_assignable::value && + !std::is_abstract::value; + }; + + template + typename std::enable_if::value>::type + assign_exception(Ex& e) { + assign_eptr(e); + } + + template + typename std::enable_if::value>::type + assign_exception(Ex& e) { + this->item_ = std::make_shared(e); + this->throwfn_ = folly::detail::Thrower::doThrow; + } + template void call_fn(F&& fn) { try { Base::call_fn(std::move(fn)); - } catch (const LastException& e) { - this->item_ = std::make_shared(e); - this->throwfn_ = folly::detail::Thrower::doThrow; + } catch (LastException& e) { + if (typeid(e) == typeid(LastException&)) { + assign_exception(e); + } else { + assign_eptr(e); + } } } }; diff --git a/folly/String.h b/folly/String.h index 0b2b8646..223c4907 100644 --- a/folly/String.h +++ b/folly/String.h @@ -349,8 +349,15 @@ std::string hexDump(const void* ptr, size_t size); fbstring errnoStr(int err); /** - * Debug string for an exception: include type and what(). + * Debug string for an exception: include type and what(). Note that + * the non-templated function overloads will be used in preference to + * the template. */ +template +fbstring exceptionStr(const T& e) { + return folly::to(demangle(typeid(e))); +} + inline fbstring exceptionStr(const std::exception& e) { return folly::to(demangle(typeid(e)), ": ", e.what()); } diff --git a/folly/test/ExceptionWrapperBenchmark.cpp b/folly/test/ExceptionWrapperBenchmark.cpp index e7895198..e384be65 100644 --- a/folly/test/ExceptionWrapperBenchmark.cpp +++ b/folly/test/ExceptionWrapperBenchmark.cpp @@ -44,7 +44,7 @@ BENCHMARK_RELATIVE(exception_wrapper_create_and_test, iters) { std::runtime_error e("payload"); for (int i = 0; i < iters; ++i) { auto ew = folly::make_exception_wrapper(e); - assert(ew.get()); + assert(ew); } } @@ -81,7 +81,7 @@ BENCHMARK_RELATIVE(exception_wrapper_create_and_test_concurrent, iters) { std::runtime_error e("payload"); for (int i = 0; i < iters; ++i) { auto ew = folly::make_exception_wrapper(e); - assert(ew.get()); + assert(ew); } }); } @@ -127,9 +127,7 @@ BENCHMARK_RELATIVE(exception_wrapper_create_and_cast, iters) { std::runtime_error e("payload"); for (int i = 0; i < iters; ++i) { auto ew = folly::make_exception_wrapper(e); - std::exception* basePtr = static_cast(ew.get()); - auto ep = dynamic_cast(basePtr); - assert(ep); + assert(ew.is_compatible_with()); } } @@ -196,9 +194,7 @@ BENCHMARK_RELATIVE(exception_wrapper_create_and_cast_concurrent, iters) { std::runtime_error e("payload"); for (int i = 0; i < iters; ++i) { auto ew = folly::make_exception_wrapper(e); - std::exception* basePtr = static_cast(ew.get()); - auto ep = dynamic_cast(basePtr); - assert(ep); + assert(ew.is_compatible_with()); } }); } diff --git a/folly/test/ExceptionWrapperTest.cpp b/folly/test/ExceptionWrapperTest.cpp index 8683e89f..e04b9f47 100644 --- a/folly/test/ExceptionWrapperTest.cpp +++ b/folly/test/ExceptionWrapperTest.cpp @@ -17,6 +17,7 @@ #include #include #include +#include using namespace folly; @@ -52,9 +53,10 @@ TEST(ExceptionWrapper, try_and_catch_test) { [=]() { throw std::runtime_error(expected); }); - EXPECT_TRUE(ew.get()); - EXPECT_EQ(ew.get()->what(), expected); - auto rep = dynamic_cast(ew.get()); + EXPECT_TRUE(bool(ew)); + EXPECT_TRUE(ew.getCopied()); + EXPECT_EQ(ew.what(), "std::runtime_error: payload"); + auto rep = ew.is_compatible_with(); EXPECT_TRUE(rep); // Changing order is like catching in wrong order. Beware of this in your @@ -62,19 +64,20 @@ TEST(ExceptionWrapper, try_and_catch_test) { auto ew2 = try_and_catch([=]() { throw std::runtime_error(expected); }); - EXPECT_TRUE(ew2.get()); + EXPECT_TRUE(bool(ew2)); // We are catching a std::exception, not std::runtime_error. - EXPECT_NE(ew2.get()->what(), expected); - rep = dynamic_cast(ew2.get()); - EXPECT_FALSE(rep); + EXPECT_FALSE(ew2.getCopied()); + // But, we can still get the actual type if we want it. + rep = ew2.is_compatible_with(); + EXPECT_TRUE(rep); // Catches even if not rightmost. auto ew3 = try_and_catch([]() { throw std::exception(); }); - EXPECT_TRUE(ew3.get()); - EXPECT_NE(ew3.get()->what(), expected); - rep = dynamic_cast(ew3.get()); + EXPECT_TRUE(bool(ew3)); + EXPECT_NE(ew3.what(), expected); + rep = ew3.is_compatible_with(); EXPECT_FALSE(rep); // If does not catch, throws. @@ -84,3 +87,71 @@ TEST(ExceptionWrapper, try_and_catch_test) { }), std::exception); } + +class AbstractIntException : public std::exception { +public: + virtual int getInt() const = 0; +}; + +class IntException : public AbstractIntException { +public: + explicit IntException(int i) + : i_(i) {} + + virtual int getInt() const { return i_; } + virtual const char* what() const noexcept override { + what_ = folly::to("int == ", i_); + return what_.c_str(); + } + +private: + int i_; + mutable std::string what_; +}; + +TEST(ExceptionWrapper, with_exception_test) { + int expected = 23; + + // This works, and doesn't slice. + exception_wrapper ew = try_and_catch( + [=]() { + throw IntException(expected); + }); + EXPECT_TRUE(bool(ew)); + EXPECT_EQ(ew.what(), "IntException: int == 23"); + 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. + exception_wrapper ew2 = try_and_catch( + [=]() { + throw IntException(expected); + }); + EXPECT_TRUE(bool(ew2)); + EXPECT_EQ(ew2.what(), "IntException: int == 23"); + ew2.with_exception([&](AbstractIntException& ie) { + EXPECT_EQ(ie.getInt(), expected); + EXPECT_EQ(typeid(ie), typeid(IntException)); + }); +} + +TEST(ExceptionWrapper, non_std_exception_test) { + int expected = 17; + + exception_wrapper ew = try_and_catch( + [=]() { + throw expected; + }); + EXPECT_TRUE(bool(ew)); + EXPECT_FALSE(ew.is_compatible_with()); + EXPECT_EQ(ew.what(), "int"); + // non-std::exception types are supported, but the only way to + // access their value is to explicity rethrow and catch it. + try { + ew.throwException(); + } catch (int& i) { + EXPECT_EQ(i, expected); + } +}