-
Type: Bug
-
Resolution: Fixed
-
Priority: Major - P3
-
Affects Version/s: None
-
Component/s: Internal Code
-
Fully Compatible
-
ALL
-
v5.1, v5.0
-
-
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:
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.
- is caused by
-
SERVER-54408 Rewrite AsyncTry-until to not use future recursion
- Closed
- related to
-
SERVER-89900 Shutdown and join the executor in resharding_txn_cloner_test.cpp on teardown
- Closed