Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-53500

Investigate making nested futures equivalent to chained continuations

    • Service Arch
    • 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.

            Assignee:
            backlog-server-servicearch [DO NOT USE] Backlog - Service Architecture
            Reporter:
            max.hirschhorn@mongodb.com Max Hirschhorn
            Votes:
            1 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated: