-
Type: Task
-
Resolution: Unresolved
-
Priority: Major - P3
-
None
-
Affects Version/s: None
-
Component/s: Internal Code
-
Server Programmability
-
7
It is somewhat easy to misuse mongo::Futures by having them get chained through nesting (e.g. a future returned by another function) rather than chained at the top-level. Nesting futures can lead to hitting the kMaxDepth invariant (--dbg=on only) and it isn't immediately obvious to consumers of futures why a different syntax could have significantly different behavior.
Unable to find source-code formatter for language: diff. Available languages are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml
diff --git a/src/mongo/util/future_util.h b/src/mongo/util/future_util.h index ff846fabc3..d1891e8216 100644 --- a/src/mongo/util/future_util.h +++ b/src/mongo/util/future_util.h @@ -263,15 +263,17 @@ private: // If the request is already canceled, don't run anything. if (cancelToken.isCanceled()) return ExecutorFuture<ReturnType>(executor, asyncTryCanceledStatus()); - auto future = ExecutorFuture<void>(executor).then(executeLoopBody); - return std::move(future).onCompletion( - [this, self = this->shared_from_this()](StatusOrStatusWith<ReturnType> s) { - if (shouldStopIteration(s)) - return ExecutorFuture<ReturnType>(executor, std::move(s)); - - return run(); - }); + return ExecutorFuture<void>(executor).then([this, self = this->shared_from_this()] { + return ExecutorFuture<void>(executor) + .then([this, self = this->shared_from_this()] { return executeLoopBody(); }) + .onCompletion( + [this, self = this->shared_from_this()](StatusOrStatusWith<ReturnType> s) { + if (shouldStopIteration(s)) + return ExecutorFuture<ReturnType>(executor, std::move(s)); + return run(); + }); + }); } std::shared_ptr<executor::TaskExecutor> executor; diff --git a/src/mongo/util/future_util_test.cpp b/src/mongo/util/future_util_test.cpp index e9878d21e8..731b4505f6 100644 --- a/src/mongo/util/future_util_test.cpp +++ b/src/mongo/util/future_util_test.cpp @@ -35,6 +35,7 @@ #include "mongo/executor/thread_pool_task_executor_test_fixture.h" #include "mongo/unittest/barrier.h" #include "mongo/unittest/death_test.h" +#include "mongo/util/concurrency/thread_pool.h" #include "mongo/util/future_util.h" namespace mongo { @@ -46,7 +47,10 @@ public: auto network = std::make_unique<executor::NetworkInterfaceMock>(); _network = network.get(); - _executor = makeSharedThreadPoolTestExecutor(std::move(network)); + ThreadPool::Options threadPoolOptions; + threadPoolOptions.maxThreads = 10; + _executor = std::make_shared<executor::ThreadPoolTaskExecutor>( + std::make_unique<ThreadPool>(std::move(threadPoolOptions)), std::move(network)); _executor->startup(); } @@ -109,6 +113,22 @@ TEST_F(AsyncTryUntilTest, LoopExecutesUntilConditionIsTrue) { ASSERT_EQ(i, numLoops); } +TEST_F(AsyncTryUntilTest, LoopReturnsFutureAndExecutesUntilConditionIsTrue) { + const int numLoops = 3000; + auto i = 0; + auto resultFut = AsyncTry([&] { + return makeReadyFutureWith([&] { + ++i; + return i; + }); + }) + .until([&](StatusWith<int> swInt) { return swInt.getValue() == numLoops; }) + .on(executor(), CancelationToken::uncancelable()); + resultFut.wait(); + + ASSERT_EQ(i, numLoops); +} + TEST_F(AsyncTryUntilTest, LoopDoesNotRespectConstDelayIfConditionIsAlreadyTrue) { auto i = 0; auto resultFut = AsyncTry([&] { ++i; })
Acceptance criteria:
- We have understood and documented the conditions under which the max depth invariant can be hit, including how the isJustForContinuation optimization works and what the execution graph looks like in different cases
- Outline list of proposed optimizations for different future chains/tree structures such that we can avoid hitting this invariant, or if not possible, a proposal to make it safe to remove the invariant. Specifically, we should be able to do nested async loops in a standard way without hitting this invariant.
- related to
-
SERVER-54119 Destroy SharedStateBase as we unfold continuations
- Closed
-
SERVER-54408 Rewrite AsyncTry-until to not use future recursion
- Closed