refactor: add internal Promise/Future classes for future use#521
refactor: add internal Promise/Future classes for future use#521
Conversation
|
The Windows CI seems to be broken because it couldn't download OpenSSL? I don't think that has anything to do with my change. |
|
FWIW, I wrote the bulk of this code by hand. Claude helped fill out the comments, and also helped me work through some of the trickier template syntax. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7fa60a3. Configure here.
| // Call continuations outside the lock so that continuations which | ||
| // re-enter this future (e.g. via GetResult or Then) don't deadlock. | ||
| for (auto& continuation : to_call) { | ||
| continuation(*result_); |
There was a problem hiding this comment.
Should we copy the result to a local under the lock?
bool Resolve(T result) {
...
T resolved_value; // local copy
{
...
resolved_value = *result_; // copy under the lock
...
}
for (auto& continuation : to_call) {
continuation(resolved_value); // use local copy
}
return true;
}
| // The continuation ignores T entirely (returning a dummy int). The | ||
| // executor signals the cv when called, since being called means the | ||
| // original future has resolved. | ||
| Then([](T const&) { return 0; }, |
There was a problem hiding this comment.
Would it make sense to have something similar to an OnResolved that doesn't need to create the inner Promise/Future?
Probably not I guess.
| template <typename T> | ||
| class Future { | ||
| public: | ||
| Future(std::shared_ptr<PromiseInternal<T>> internal) |
There was a problem hiding this comment.
Should we be doing the fiend-class dance to enforce use of ::GetFuture? Internal, so probably not critical.
| class Promise { | ||
| public: | ||
| Promise() : internal_(std::make_shared<PromiseInternal<T>>()) {} | ||
| ~Promise() = default; |
There was a problem hiding this comment.
Do we need any Future handling for an abandoned unresolved promise?
| } | ||
|
|
||
| // Move result into storage if possible; otherwise copy. | ||
| if constexpr (std::is_move_assignable_v<T>) { |
| std::shared_ptr<PromiseInternal<T>> internal_; | ||
| }; | ||
|
|
||
| // WhenAll takes a variadic list of Futures (each with potentially different |
There was a problem hiding this comment.
Should we clarify the executor here?
There was a problem hiding this comment.
Do we need some more thread-intense test scenarios?

This PR adds internal-only
PromiseandFutureclasses. These have some advantages over using callbacks everywhere:WhenAllandWhenAnyonly have to be written once, instead of having to re-invent the wheel every time we need those concepts.std::future, this supports aThenmethod, which enables things likeWhenAnywithout occupying an extra thread per future.std::function, theContinuationclass used in this implementation supports move-only functions.Some points to consider:
std::promise,std::future, andthen()(experimental).Then, because there is no natural default:asio::post, as this class may be used for other things.Future, and encourage users to useFuture<std::expected<>>if they care about it, to keep things simpler.GetResultis non-blocking, as this prevents unintended blocking. There is a blocking version calledWaitForResultwith a timeout if needed.Thenare passed the result of theFutureinstead of theFutureitself. This is simpler, but it means we can't add more metadata in the future without a breaking change. This is an internal class, so I think it's better to keep it simple.TinPromise<T>/Future<T>has to be copyable. This is an inherent limitation of the need to be able to haveFuture<Future<T>>unwrappable toFuture<T>, while also letting aFuturehave multiple continuations.I added lots of tests. Some of them double as documentation for humans and LLMs.
Note
Low Risk
Adds new internal async primitives and tests without changing existing runtime behavior, but introduces concurrency and executor-based chaining logic that could be subtle if adopted later.
Overview
Adds an internal
launchdarkly::asyncpromise/future implementation (Promise,Future) with move-onlyContinuationand executor-drivenThenchaining, including support for flattening when a continuation returns anotherFuture.Introduces
WhenAllandWhenAnycombinators plusWaitForResult(timeout-based blocking helper), and updates the internal CMake header glob to install/build newinclude/launchdarkly/async/*.hppheaders. A comprehensive newpromise_test.cppcovers basic chaining, ASIO scheduling, move/copy-only captures,tl::expectedusage, andWhenAll/WhenAnybehavior.Reviewed by Cursor Bugbot for commit dec560f. Bugbot is set up for automated code reviews on this repo. Configure here.