Skip to content

refactor: add internal Promise/Future classes for future use#521

Open
beekld wants to merge 14 commits intomainfrom
beeklimt/promises
Open

refactor: add internal Promise/Future classes for future use#521
beekld wants to merge 14 commits intomainfrom
beeklimt/promises

Conversation

@beekld
Copy link
Copy Markdown
Contributor

@beekld beekld commented Apr 13, 2026

This PR adds internal-only Promise and Future classes. These have some advantages over using callbacks everywhere:

  • They are more easily composable. That means utility functions like WhenAll and WhenAny only have to be written once, instead of having to re-invent the wheel every time we need those concepts.
  • This allows C++ code to be structured more like the code in other languages with proper promise types.
  • Unlike std::future, this supports a Then method, which enables things like WhenAny without occupying an extra thread per future.
  • Unlike std::function, the Continuation class used in this implementation supports move-only functions.

Some points to consider:

  • Would other people like using these, or am I the only one?
  • Naming: I chose to mimic the C++ names of std::promise, std::future, and then() (experimental).
  • Executors: I chose to require passing an executor into Then, because there is no natural default:
    • C++ doesn't have a "main" thread exactly, or at least, it's not used the same way as on other platforms.
    • Immediate executors are confusing to reason about, and result in calling work on unpredictable threads.
    • We shouldn't assume asio::post, as this class may be used for other things.
    • I didn't want to add the complexity of storing execution contexts in a thread-local.
  • Error handling: I chose not to put error handling directly in the Future, and encourage users to use Future<std::expected<>> if they care about it, to keep things simpler.
  • Cancellation: I chose not to implement cancellation. We can add it in the future. But cancellation can be treated as mostly orthogonal to futures, and we can't cancel http requests currently anyway, so 🤷‍♀️ .
  • By default, GetResult is non-blocking, as this prevents unintended blocking. There is a blocking version called WaitForResult with a timeout if needed.
  • The continuations in Then are passed the result of the Future instead of the Future itself. 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.
  • The type T in Promise<T>/Future<T> has to be copyable. This is an inherent limitation of the need to be able to have Future<Future<T>> unwrappable to Future<T>, while also letting a Future have multiple continuations.
  • The executor has to be copyable, for simplicity. ASIO has the same requirement on executors, and it seems fine.

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::async promise/future implementation (Promise, Future) with move-only Continuation and executor-driven Then chaining, including support for flattening when a continuation returns another Future.

Introduces WhenAll and WhenAny combinators plus WaitForResult (timeout-based blocking helper), and updates the internal CMake header glob to install/build new include/launchdarkly/async/*.hpp headers. A comprehensive new promise_test.cpp covers basic chaining, ASIO scheduling, move/copy-only captures, tl::expected usage, and WhenAll/WhenAny behavior.

Reviewed by Cursor Bugbot for commit dec560f. Bugbot is set up for automated code reviews on this repo. Configure here.

@beekld beekld requested a review from a team as a code owner April 13, 2026 20:48
@beekld
Copy link
Copy Markdown
Contributor Author

beekld commented Apr 13, 2026

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.

@beekld
Copy link
Copy Markdown
Contributor Author

beekld commented Apr 13, 2026

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.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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_);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this check?

std::shared_ptr<PromiseInternal<T>> internal_;
};

// WhenAll takes a variadic list of Futures (each with potentially different
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we clarify the executor here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need some more thread-intense test scenarios?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants