From bcbc4ff50e1949a01a102faf90f33b4f5dbe3263 Mon Sep 17 00:00:00 2001 From: Ameya Limaye Date: Wed, 8 Feb 2017 11:23:24 -0800 Subject: [PATCH] Throw custom exception type when ABD fails a result because some other token did not call dispatch Summary: Throw custom exception type when ABD fails a result because some other token did not call dispatch: - This is useful because the caller can catch this exception and discard it if they find some other exception - The custom exception does not give an indication of the problem. The other exception/s thrown will do that. Reviewed By: yfeldblum Differential Revision: D4395742 fbshipit-source-id: be80f66b1297e9faf625a2fb087590a7d0a9335d --- folly/Makefile.am | 4 +- folly/fibers/AtomicBatchDispatcher-inl.h | 87 ++++++++++--------- folly/fibers/AtomicBatchDispatcher.h | 47 ++++++++-- folly/fibers/detail/AtomicBatchDispatcher.cpp | 55 ++++++++++++ folly/fibers/detail/AtomicBatchDispatcher.h | 34 ++++++++ folly/fibers/test/FibersTest.cpp | 12 +-- 6 files changed, 185 insertions(+), 54 deletions(-) create mode 100644 folly/fibers/detail/AtomicBatchDispatcher.cpp create mode 100644 folly/fibers/detail/AtomicBatchDispatcher.h diff --git a/folly/Makefile.am b/folly/Makefile.am index eedc1dd0..7e5a0284 100644 --- a/folly/Makefile.am +++ b/folly/Makefile.am @@ -79,7 +79,7 @@ nobase_follyinclude_HEADERS = \ detail/StaticSingletonManager.h \ detail/Stats.h \ detail/ThreadLocalDetail.h \ - detail/TryDetail.h \ + detail/TryDetail.h \ detail/TurnSequencer.h \ detail/UncaughtExceptionCounter.h \ Demangle.h \ @@ -550,6 +550,7 @@ nobase_follyinclude_HEADERS += \ fibers/Baton-inl.h \ fibers/BatchDispatcher.h \ fibers/BoostContextCompatibility.h \ + fibers/detail/AtomicBatchDispatcher.h \ fibers/EventBaseLoopController.h \ fibers/EventBaseLoopController-inl.h \ fibers/Fiber.h \ @@ -577,6 +578,7 @@ nobase_follyinclude_HEADERS += \ libfolly_la_SOURCES += \ fibers/Baton.cpp \ + fibers/detail/AtomicBatchDispatcher.cpp \ fibers/Fiber.cpp \ fibers/FiberManager.cpp \ fibers/FiberManagerMap.cpp \ diff --git a/folly/fibers/AtomicBatchDispatcher-inl.h b/folly/fibers/AtomicBatchDispatcher-inl.h index 7d0b1d43..549112f3 100644 --- a/folly/fibers/AtomicBatchDispatcher-inl.h +++ b/folly/fibers/AtomicBatchDispatcher-inl.h @@ -29,12 +29,14 @@ struct AtomicBatchDispatcher::DispatchBaton { optEntries_.reserve(numEntries); } - void setError(std::string message) { - optErrorMessage_ = std::move(message); + void setExceptionWrapper(folly::exception_wrapper&& exWrapper) { + exceptionWrapper_ = std::move(exWrapper); } void setExpectedCount(size_t expectedCount) { + assert(expectedCount_ == 0 || !"expectedCount_ being set more than once"); expectedCount_ = expectedCount; + optEntries_.resize(expectedCount_); } Future getFutureResult(InputT&& input, size_t sequenceNumber) { @@ -42,17 +44,13 @@ struct AtomicBatchDispatcher::DispatchBaton { optEntries_.resize(sequenceNumber + 1); } folly::Optional& optEntry = optEntries_[sequenceNumber]; - if (optEntry) { - throw std::logic_error( - "Cannot have multiple inputs with same token sequence number"); - } + assert(!optEntry || !"Multiple inputs have the same token sequence number"); optEntry = Entry(std::move(input)); return optEntry->promise.getFuture(); } private: - void setExceptionResults(std::exception_ptr eptr) { - auto exceptionWrapper = exception_wrapper(eptr); + void setExceptionResults(const folly::exception_wrapper& exceptionWrapper) { for (auto& optEntry : optEntries_) { if (optEntry) { optEntry->promise.setException(exceptionWrapper); @@ -60,53 +58,56 @@ struct AtomicBatchDispatcher::DispatchBaton { } } + void setExceptionResults(std::exception_ptr eptr) { + auto exceptionWrapper = exception_wrapper(eptr); + return setExceptionResults(exceptionWrapper); + } + template void setExceptionResults( const TException& ex, std::exception_ptr eptr = std::exception_ptr()) { auto exceptionWrapper = eptr ? exception_wrapper(eptr, ex) : exception_wrapper(ex); - for (auto& optEntry : optEntries_) { - if (optEntry) { - optEntry->promise.setException(exceptionWrapper); - } - } + return setExceptionResults(exceptionWrapper); } void fulfillPromises() { try { // If an error message is set, set all promises to exception with message - if (optErrorMessage_) { - auto ex = std::logic_error(*optErrorMessage_); - return setExceptionResults(std::move(ex)); + if (exceptionWrapper_) { + return setExceptionResults(exceptionWrapper_); } - // Create inputs vector and validate entries count same as expectedCount_ - std::vector inputs; - inputs.reserve(expectedCount_); - bool allEntriesFound = (optEntries_.size() == expectedCount_); - if (allEntriesFound) { - for (auto& optEntry : optEntries_) { - if (!optEntry) { - allEntriesFound = false; - break; - } - inputs.emplace_back(std::move(optEntry->input)); + // Validate entries count same as expectedCount_ + assert( + optEntries_.size() == expectedCount_ || + !"Entries vector did not have expected size"); + std::vector vecTokensNotDispatched; + for (size_t i = 0; i < expectedCount_; ++i) { + if (!optEntries_[i]) { + vecTokensNotDispatched.push_back(i); } } - if (!allEntriesFound) { - auto ex = std::logic_error( - "One or more input tokens destroyed before calling dispatch"); - return setExceptionResults(std::move(ex)); + if (!vecTokensNotDispatched.empty()) { + return setExceptionResults(ABDTokenNotDispatchedException( + detail::createABDTokenNotDispatchedExMsg(vecTokensNotDispatched))); + } + + // Create the inputs vector + std::vector inputs; + inputs.reserve(expectedCount_); + for (auto& optEntry : optEntries_) { + inputs.emplace_back(std::move(optEntry->input)); } // Call the user provided batch dispatch function to get all results // and make sure that we have the expected number of results returned auto results = dispatchFunction_(std::move(inputs)); if (results.size() != expectedCount_) { - auto ex = std::logic_error( - "Unexpected number of results returned from dispatch function"); - return setExceptionResults(std::move(ex)); + return setExceptionResults( + ABDUsageException(detail::createUnexpectedNumResultsABDUsageExMsg( + expectedCount_, results.size()))); } // Fulfill the promises with the results from the batch dispatch @@ -114,8 +115,10 @@ struct AtomicBatchDispatcher::DispatchBaton { optEntries_[i]->promise.setValue(std::move(results[i])); } } catch (const std::exception& ex) { + // Set exceptions thrown when executing the user provided dispatch func return setExceptionResults(ex, std::current_exception()); } catch (...) { + // Set exceptions thrown when executing the user provided dispatch func return setExceptionResults(std::current_exception()); } } @@ -139,7 +142,7 @@ struct AtomicBatchDispatcher::DispatchBaton { size_t expectedCount_; DispatchFunctionT dispatchFunction_; std::vector> optEntries_; - folly::Optional optErrorMessage_; + folly::exception_wrapper exceptionWrapper_; }; template @@ -158,7 +161,7 @@ Future AtomicBatchDispatcher::Token::dispatch( InputT input) { auto baton = std::move(baton_); if (!baton) { - throw std::logic_error( + throw ABDUsageException( "Dispatch called more than once on the same Token object"); } return baton->getFutureResult(std::move(input), sequenceNumber_); @@ -173,8 +176,10 @@ AtomicBatchDispatcher::AtomicBatchDispatcher( template AtomicBatchDispatcher::~AtomicBatchDispatcher() { if (baton_) { - baton_->setError( - "AtomicBatchDispatcher destroyed before commit() was called on it"); + // Set error here rather than throw because we do not want to throw from + // the destructor of AtomicBatchDispatcher + baton_->setExceptionWrapper( + folly::make_exception_wrapper()); commit(); } } @@ -182,7 +187,7 @@ AtomicBatchDispatcher::~AtomicBatchDispatcher() { template void AtomicBatchDispatcher::reserve(size_t numEntries) { if (!baton_) { - throw std::logic_error("Cannot call reserve(....) after calling commit()"); + throw ABDUsageException("Cannot call reserve(....) after calling commit()"); } baton_->reserve(numEntries); } @@ -190,7 +195,7 @@ void AtomicBatchDispatcher::reserve(size_t numEntries) { template auto AtomicBatchDispatcher::getToken() -> Token { if (!baton_) { - throw std::logic_error("Cannot issue more tokens after calling commit()"); + throw ABDUsageException("Cannot issue more tokens after calling commit()"); } return Token(baton_, numTokensIssued_++); } @@ -199,7 +204,7 @@ template void AtomicBatchDispatcher::commit() { auto baton = std::move(baton_); if (!baton) { - throw std::logic_error( + throw ABDUsageException( "Cannot call commit() more than once on the same dispatcher"); } baton->setExpectedCount(numTokensIssued_); diff --git a/folly/fibers/AtomicBatchDispatcher.h b/folly/fibers/AtomicBatchDispatcher.h index ebe52fac..93f9c93b 100644 --- a/folly/fibers/AtomicBatchDispatcher.h +++ b/folly/fibers/AtomicBatchDispatcher.h @@ -17,15 +17,50 @@ #include #include +#include #include #include #include +#include +#include #include #include namespace folly { namespace fibers { +/** + * An exception class that gets thrown when the AtomicBatchDispatcher is used + * incorrectly. This is indicative of a bug in the user code. + * Examples are, multiple dispatch calls on the same token, trying to get more + * tokens from the dispatcher after commit has been called, etc. + */ +class ABDUsageException : public std::logic_error { + using std::logic_error::logic_error; +}; + +/** + * An exception class that gets set on the promise for dispatched tokens, when + * the AtomicBatchDispatcher was destroyed before commit was called on it. + */ +class ABDCommitNotCalledException : public std::runtime_error { + public: + ABDCommitNotCalledException() + : std::runtime_error( + "AtomicBatchDispatcher destroyed before commit() was called") {} +}; + +/** + * An exception class that gets set on the promise for dispatched tokens, when + * one or more other tokens in the batch were destroyed before dispatch was + * called on them. + * Only here so that the caller can distinguish the real failure cause + * rather than these subsequently thrown exceptions. + */ +class ABDTokenNotDispatchedException : public std::runtime_error { + using std::runtime_error::runtime_error; +}; + /** * AtomicBatchDispatcher should be used if you want to process fiber tasks in * parallel, but require to synchronize them at some point. The canonical @@ -72,21 +107,21 @@ namespace fibers { * 1) The dispatcher is destroyed before calling commit on it, for example * because the user forgot to call commit OR an exception was thrown * in user code before the call to commit: - * - The future ResultT has an exception of type std::logic_error set for all - * tokens that were issued by the dispatcher (once all tokens are either - * destroyed or have called dispatch) + * - The future ResultT has an exception of type ABDCommitNotCalledException + * set for all tokens that were issued by the dispatcher (once all tokens + * are either destroyed or have called dispatch) * 2) Calling the dispatch function more than once on the same Token object * (or a moved version of the same Token): * - Subsequent calls to dispatch (after the first one) will throw an - * std::logic_error exception (the batch itself will not have any errors + * ABDUsageException exception (the batch itself will not have any errors * and will get processed) * 3) One/more of the Tokens issued are destroyed before calling dispatch on * it/them: - * - The future ResultT has an exception of type std::logic_error set for all + * - The future ResultT has an ABDTokenNotDispatchedException set for all * tokens that were issued by the dispatcher (once all tokens are either * destroyed or have called dispatch) * 4) dispatcher.getToken() is called after calling dispatcher.commit() - * - the call to getToken() will throw an std::logic_error exception + * - the call to getToken() will throw an ABDUsageException exception * (the batch itself will not have any errors and will get processed). * 5) All tokens were issued and called dispatch, the user provided batch * dispatch function is called, but that function throws any exception. diff --git a/folly/fibers/detail/AtomicBatchDispatcher.cpp b/folly/fibers/detail/AtomicBatchDispatcher.cpp new file mode 100644 index 00000000..37d37347 --- /dev/null +++ b/folly/fibers/detail/AtomicBatchDispatcher.cpp @@ -0,0 +1,55 @@ +/* + * Copyright 2017-present Facebook, Inc. + * + * Licensed 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. + */ +#include "AtomicBatchDispatcher.h" +#include + +namespace folly { +namespace fibers { +namespace detail { + +std::string createABDTokenNotDispatchedExMsg( + const std::vector& vecTokensNotDispatched) { + size_t numTokensNotDispatched = vecTokensNotDispatched.size(); + assert(numTokensNotDispatched > 0); + size_t numSeqNumToPrint = + (numTokensNotDispatched > 10 ? 10 : numTokensNotDispatched); + std::string strInputsNotFound = + folly::sformat("{}", vecTokensNotDispatched[0]); + for (size_t i = 1; i < numSeqNumToPrint; ++i) { + strInputsNotFound += folly::sformat(", {}", vecTokensNotDispatched[i]); + } + if (numSeqNumToPrint < numTokensNotDispatched) { + strInputsNotFound += "..."; + } + return folly::sformat( + "{} input tokens (seq nums: {}) destroyed before calling dispatch", + numTokensNotDispatched, + strInputsNotFound); +} + +std::string createUnexpectedNumResultsABDUsageExMsg( + size_t numExpectedResults, + size_t numActualResults) { + return folly::sformat( + "Unexpected number of results ({}) returned from dispatch function, " + "expected ({})", + numActualResults, + numExpectedResults); +} + +} // namespace detail +} // namespace fibers +} // namespace folly diff --git a/folly/fibers/detail/AtomicBatchDispatcher.h b/folly/fibers/detail/AtomicBatchDispatcher.h new file mode 100644 index 00000000..14c7190f --- /dev/null +++ b/folly/fibers/detail/AtomicBatchDispatcher.h @@ -0,0 +1,34 @@ +/* + * Copyright 2017 Facebook, Inc. + * + * Licensed 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. + */ +#pragma once + +#include +#include + +namespace folly { +namespace fibers { +namespace detail { + +std::string createABDTokenNotDispatchedExMsg( + const std::vector& vecTokensNotDispatched); + +std::string createUnexpectedNumResultsABDUsageExMsg( + size_t numExpectedResults, + size_t numActualResults); + +} // namespace detail +} // namespace fibers +} // namespace folly diff --git a/folly/fibers/test/FibersTest.cpp b/folly/fibers/test/FibersTest.cpp index fd180173..af3fe75a 100644 --- a/folly/fibers/test/FibersTest.cpp +++ b/folly/fibers/test/FibersTest.cpp @@ -1864,7 +1864,7 @@ void dispatchJobs( if (dispatchProblem == DispatchProblem::DuplicateDispatch) { if (i == problemIndex) { - EXPECT_THROW(job.token.dispatch(job.input), std::logic_error); + EXPECT_THROW(job.token.dispatch(job.input), ABDUsageException); } } } catch (...) { @@ -1969,7 +1969,7 @@ TEST(FiberManager, ABD_DispatcherDestroyedBeforeCallingCommit) { /* User code handles the exception and does not exit process */ } evb.loop(); - validateResults(results, COUNT); + validateResults(results, COUNT); } TEST(FiberManager, ABD_PreprocessingFailureTest) { @@ -1985,7 +1985,7 @@ TEST(FiberManager, ABD_PreprocessingFailureTest) { dispatchJobs(executor, jobs, results, DispatchProblem::PreprocessThrows, 8); atomicBatchDispatcher.commit(); evb.loop(); - validateResults(results, COUNT - 1); + validateResults(results, COUNT - 1); } TEST(FiberManager, ABD_MultipleDispatchOnSameTokenErrorTest) { @@ -2014,12 +2014,12 @@ TEST(FiberManager, ABD_GetTokenCalledAfterCommitTest) { createAtomicBatchDispatcher(std::move(dispatchFunc)); createJobs(atomicBatchDispatcher, jobs, COUNT); atomicBatchDispatcher.commit(); - EXPECT_THROW(atomicBatchDispatcher.getToken(), std::logic_error); + EXPECT_THROW(atomicBatchDispatcher.getToken(), ABDUsageException); dispatchJobs(executor, jobs, results); - EXPECT_THROW(atomicBatchDispatcher.getToken(), std::logic_error); + EXPECT_THROW(atomicBatchDispatcher.getToken(), ABDUsageException); evb.loop(); validateResults(results, COUNT); - EXPECT_THROW(atomicBatchDispatcher.getToken(), std::logic_error); + EXPECT_THROW(atomicBatchDispatcher.getToken(), ABDUsageException); } TEST(FiberManager, ABD_UserProvidedBatchDispatchThrowsTest) { -- 2.34.1