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

TryUntilLoop Does Not Synchronize Destructor and Promise resolution

    XMLWordPrintable

    Details

    • Operating System:
      ALL
    • Steps To Reproduce:
      Hide

      Known Failing Commit - 8ef4580520429754f85003cdb9d126aede402dbd

      Replicating Code Diff

      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 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
    • Sprint:
      Service Arch 2021-10-04
    • Linked BF Score:
      129
    • Story Points:
      2

      Description

      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:

      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.

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              amirsaman.memaripour Amirsaman Memaripour
              Reporter:
              luis.osta Luis Osta
              Participants:
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated: