Skip to content

refactor: implement fdv2 polling initializer / synchronizer#519

Merged
beekld merged 39 commits intomainfrom
beeklimt/SDK-2097
Apr 21, 2026
Merged

refactor: implement fdv2 polling initializer / synchronizer#519
beekld merged 39 commits intomainfrom
beeklimt/SDK-2097

Conversation

@beekld
Copy link
Copy Markdown
Contributor

@beekld beekld commented Apr 3, 2026

This implements the C++ polling initializer and synchronizer.

I have rewritten this code substantially since the first round of reviews, mostly by hand.

Changes:

  • Implements FDv2PollingInitializer and FDv2PollingSynchronizer.
  • Updates the IFDv2Initializer and IFDv2Synchronizer interfaces to be async.
  • Makes it safe to delete both classes without joining any threads.
  • Updates AsioRequester::Request to use a more modern style that is more flexible in how it can be used.
    • This will be needed for cancellation eventually anyway.
  • Marks a method in AsioRequester as const to make it more obvious that it's thread-safe.
  • Adds a helper to create promisified ASIO timers.
  • Adds some minor Promise helpers to simplify the new code.

Caveats:

  • I have not yet implemented any kind of cancellation on async operations. We should at least do this for timers at some point, but I'd rather defer it to a later PR. It's no longer a deadlock or correctness issue, but we will get timeout timers slowly piling up in the background if requests are fast.

Previous description:

The bulk of this code was generated by Claude based on the Java version, but I've gone through it line-by-line, and I think it makes sense. But I'm new to both FDv2 and ASIO, so I could be missing something.

Probably the most controversial part is the decision from the previous PR to use std::future and a blocking call. If we decide we need Java-like conditions, or if we need the callers to be non-blocking, we could change these to use asio::deferred instead. But I don't think these changes require that yet.


Note

Medium Risk
Introduces new FDv2 polling data path and converts FDv2 source interfaces to Future-based async, which can affect orchestration flow and lifecycle/shutdown behavior. Also changes the Boost.Asio HTTP requester initiation style, so regressions could impact networking behavior and timeouts.

Overview
Adds an FDv2 polling implementation: FDv2PollingInitializer performs a one-shot poll, and FDv2PollingSynchronizer repeatedly polls with minimum-interval enforcement and timeout/close handling using the internal Promise/Future utilities.

Introduces a shared FDv2ProtocolHandler state machine that accumulates FDv2 wire events into a complete FDv2ChangeSet (or emits typed protocol/JSON/server errors and Goodbye), with accompanying unit tests.

Refactors FDv2 source interfaces (IFDv2Initializer::Run, IFDv2Synchronizer::Next) to return async::Future<FDv2SourceResult> instead of blocking results, adds async helpers (kInlineExecutor, MakeFuture, async::Delay), and updates AsioRequester::Request/Requester::Request to be const and use boost::asio::async_initiate.

Reviewed by Cursor Bugbot for commit 4b02b67. 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 3, 2026 23:01
Comment thread libs/internal/src/fdv2_protocol_handler.cpp
Comment thread libs/server-sdk/src/data_systems/fdv2/polling_synchronizer.cpp
Comment thread libs/server-sdk/src/data_systems/fdv2/polling_synchronizer.cpp
Comment thread libs/server-sdk/src/data_systems/fdv2/polling_initializer.cpp
std::move(**result)}};
}
// Silently skip unknown kinds for forward-compatibility.
return std::nullopt;
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.

This ends up with maybe a slightly different layer responsibility, but I think it is likely fine. I think Java returns it, and then it is discarded a layer up, but we aren't acting on it, so it doesn't really make a difference.

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.

Actually I am a bit concerned that we may be losing granularity of data source reporting. Though maybe we don't have that concern at the moment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what exactly you're asking here. The Java version seems to have a distinction between parsing and "translation" that isn't there in C++. This function doesn't return raw JSON, so there's nothing to be returned in this case.

I believe, technically, our spec says that this case should silently ignore the uknown kind, whereas the Java implementation logs a warning. I followed the spec, but I could add a warning here, if you'd prefer.

This is the only branch that returns std::nullopt, so callers could still technically distinguish this case, but I'm not sure what else we would do about it.

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.

So, the thing I am trying to get at here is that the protocol handler here knows about the types, where I expect 1 layer up from here to know the types. Or for this to take a mapper type interface.

The reason being is that theoretically a client and server can use the same protocol handler, but they get different model types.

Server has flag and segment, and client-side has a flag-eval. The protocol itself should be agnostic to these types theoretically. Then you report "hey we got a change to kind: potato", and the data source layer can be like "I don't support potato" and then discard it.

In the Java version it just remains as some kind of JsonElement until another layer knows how to handle it.

Another option though is a mapper. Where the handler takes a mapper, and then says "map this type for me" and if the mapper says it cannot handle it, then we also know to discard it.

Mapper approach:

  const protocolHandler = internal.createProtocolHandler(
    { 'flag-eval': processFlagEval },
    config.logger,
  );

In regards to the log, I do tend to prefer a log, but one could argue about it being debug, or just once, or something. But that isn't the main concern. This layer knowing about the specific types is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am making a note to handle this in a follow-up PR. I'm just worried about this PR getting too big and out of hand.

Comment thread libs/internal/src/fdv2_protocol_handler.cpp
Comment thread libs/internal/src/fdv2_protocol_handler.cpp Outdated
Comment thread libs/server-sdk/src/data_systems/fdv2/polling_synchronizer.cpp Outdated
Comment thread libs/server-sdk/src/data_systems/fdv2/polling_initializer.cpp Outdated
Comment thread libs/server-sdk/src/data_systems/fdv2/polling_synchronizer.cpp Outdated
Comment thread libs/server-sdk/src/data_systems/fdv2/fdv2_polling_impl.cpp Outdated
Comment thread libs/internal/src/fdv2_protocol_handler.cpp
Comment thread libs/server-sdk/src/data_systems/fdv2/polling_initializer.hpp Outdated
Comment thread libs/internal/include/launchdarkly/network/asio_requester.hpp
}

boost::system::error_code ec;
auto parsed = boost::json::parse(*body, ec);
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.

Maybe we could lift the parsing into a function or have its own type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

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 112c78e. Configure here.

Comment thread libs/internal/src/fdv2_protocol_handler.cpp
@beekld beekld merged commit 1dd9793 into main Apr 21, 2026
45 checks passed
@beekld beekld deleted the beeklimt/SDK-2097 branch April 21, 2026 00:20
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