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

TryUntilLoop Does Not Synchronize Destructor and Promise resolution

    • Fully Compatible
    • ALL
    • v5.1, v5.0
    • Hide

      Known Failing Commit - 8ef4580520429754f85003cdb9d126aede402dbd

      Replicating Code Diff

      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
      index 0d5e45870e..1da2381fe8 100644
      --- a/src/mongo/util/future_util.h
      +++ b/src/mongo/util/future_util.h
      @@ -328,6 +328,7 @@ private:
                                       resultPromise.setError(asyncTryCanceledStatus());
                                   } else if (shouldStopIteration(swResult)) {
                                       resultPromise.setFrom(std::move(swResult));
      +                                sleepsecs(5);
                                   } else {
                                       runImpl(std::move(resultPromise));
                                   }
      

      And run:

      build/install/bin/db_s_shard_server_test --filter=NothingToIterate
      
      Show
      Known Failing Commit - 8ef4580520429754f85003cdb9d126aede402dbd Replicating Code Diff 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 index 0d5e45870e..1da2381fe8 100644 --- a/src/mongo/util/future_util.h +++ b/src/mongo/util/future_util.h @@ -328,6 +328,7 @@ private : resultPromise.setError(asyncTryCanceledStatus()); } else if (shouldStopIteration(swResult)) { resultPromise.setFrom(std::move(swResult)); + sleepsecs(5); } else { runImpl(std::move(resultPromise)); } And run: build/install/bin/db_s_shard_server_test --filter=NothingToIterate
    • Service Arch 2021-10-18, Service Arch 2021-11-01
    • 144
    • 2

      The current implementation of TryUntilLoop will resolve the promise and make the future ready before the function exits and hence before the destructor runs.

      This means that the member variables (executor, body, etc) are destroyed at an arbitrary time after the future is ready.

      Possible Solution:

      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 0d5e45870e..ccd57481fe 100644
      --- a/src/mongo/util/future_util.h
      +++ b/src/mongo/util/future_util.h
      @@ -298,7 +298,7 @@ private:
                   auto [promise, future] = makePromiseFuture<ReturnType>();
       
                   // Kick off the asynchronous loop.
      -            runImpl(std::move(promise));
      +            runImpl(this->shared_from_this(), std::move(promise));
       
                   return std::move(future).thenRunOn(executor);
               }
      @@ -309,30 +309,36 @@ private:
                * reschedule another iteration of the loop.
                */
               template <typename ReturnType>
      -        void runImpl(Promise<ReturnType> resultPromise) {
      -            executor->schedule(
      -                [this, self = this->shared_from_this(), resultPromise = std::move(resultPromise)](
      -                    Status scheduleStatus) mutable {
      -                    if (!scheduleStatus.isOK()) {
      -                        resultPromise.setError(std::move(scheduleStatus));
      -                        return;
      -                    }
      +        void runImpl(std::shared_ptr<TryUntilLoop> self, Promise<ReturnType> resultPromise) {
      +            invariant(self.get() == this);
      +            executor->schedule([this,
      +                                self = std::move(self),
      +                                resultPromise =
      +                                    std::move(resultPromise)](Status scheduleStatus) mutable {
      +                if (!scheduleStatus.isOK()) {
      +                    resultPromise.setError(std::move(scheduleStatus));
      +                    return;
      +                }
       
      -                    // Convert the result of the loop body into an ExecutorFuture, even if the
      -                    // loop body is not Future-returning. This isn't strictly necessary but it
      -                    // makes implementation easier.
      -                    makeExecutorFutureWith(executor, executeLoopBody)
      -                        .getAsync([this, self, resultPromise = std::move(resultPromise)](
      -                                      StatusOrStatusWith<ReturnType>&& swResult) mutable {
      +                // Convert the result of the loop body into an ExecutorFuture, even if the
      +                // loop body is not Future-returning. This isn't strictly necessary but it
      +                // makes implementation easier.
      +                makeExecutorFutureWith(executor, executeLoopBody)
      +                    .getAsync(
      +                        [this, self = std::move(self), resultPromise = std::move(resultPromise)](
      +                            StatusOrStatusWith<ReturnType>&& swResult) mutable {
                                   if (cancelToken.isCanceled()) {
                                       resultPromise.setError(asyncTryCanceledStatus());
                                   } else if (shouldStopIteration(swResult)) {
      +                                self.reset();
                                       resultPromise.setFrom(std::move(swResult));
                                   } else {
      -                                runImpl(std::move(resultPromise));
      +                                runImpl(std::move(self), std::move(resultPromise));
      +                                sleepsecs(5);
                                   }
                               });
      -                });
      +            });
               }
       
               ExecutorPtr executor;
      

      Acceptance criteria: Reinvestigate the cause of SERVER-53434 and determine whether there's a fix that relies on joining the executor in the test fixture. If not, move this ticket back to Needs Scheduling for a wider discussion with the team.

            Assignee:
            amirsaman.memaripour@mongodb.com Amirsaman Memaripour
            Reporter:
            luis.osta@mongodb.com Luis Osta (Inactive)
            Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

              Created:
              Updated:
              Resolved: