feat: Add MessageBus to core, add MakeMessageHandler to Feature#181
Conversation
|
✅ Code Quality ✅ Code Vulnerabilities ✅ Library Vulnerabilities ✅ Secrets 🎉 All green!🛠️ No new code quality issues 🔗 Commit SHA: 1a908ed | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22d0dc5db9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Start the messaging thread, which will invoke relevant message handler functions | ||
| // for each message sent on the message bus | ||
| DATADOG_ASSERT(!_message_bus_thread, "_message_bus_thread already exists on Start()"); | ||
| _message_bus_thread = std::thread( | ||
| MessagingThreadMain, std::ref(_diagnostic_logger), std::ref(*_message_bus) |
There was a problem hiding this comment.
Defer message thread until all features are started
Core::Start() launches the messaging thread before the OnCoreStarted loop, so a feature that mutates context during Start() (for example Rum::Start() dispatching UpdateContext) can enqueue ContextChangedMessage values that are delivered to handlers of later features before those features have run OnCoreStarted. In that window, handler code can observe partially initialized feature state (including unset _scope) and crash or drop required work. Please activate message delivery only after all features complete startup, or gate handlers until startup is finished.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Startup ordering fixed with ea485aa: all features will receive Start() before the messaging thread is spawned, such that a feature is guaranteed to be fully started before it receives any messages.
|
/gitlab resync-job-status |
|
View all feedbacks in Devflow UI.
15 jobs status updated and 6 skipped for a total of 21 jobs |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea485aa7d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * Called by `Core::RegisterFeature()` after the feature has been stored in the | ||
| * feature list, at which point `weak_from_this()` is safe to call. If the feature |
There was a problem hiding this comment.
Align MakeMessageHandler lifecycle contract with implementation
The new API contract says MakeMessageHandler() is invoked from Core::RegisterFeature(), but Core::Start() now calls it each time the SDK starts (core.cpp startup loop). This mismatch can lead feature authors to implement one-shot handler setup (e.g., moving state or assuming a single call), which breaks or changes behavior on restart when handlers are recreated per run. Please update this contract to reflect per-Start() invocation (or move the call site to registration).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Indeed, this documentation comment is outdated. Fixed with 1a908ed.
refs: RUM-12206
This PR builds upon #180, hooking the new
MessageBusinto theCoreand providingFeatureimplementations with the ability to respond to messages.This change allows a
Featureto react to changes inCoreContextby overridingFeature::MakeMessageHandlerand providing a callback that will handleContextChangedMessage.A feature's message handler function is executed on a new messaging thread, which reads from the Message Bus's queue and fans out each new message to all registered handler functions. This design is consistent with the iOS SDK: messages are processed asynchronously, and handling of messages does not block other threads. It's up to each feature to synchronize access to any state that's shared between the message-handler and other threads.
Out of scope:
Featureto other features.Code changes
Corenow owns aMessageBusand astd::threadthat runsMessagingThreadMainCoreContextProvidernow keeps a reference to the activeMessageBus(valid only while the SDK is running)FeatureScope::UpdateContextis processed on the context thread,CoreContextProviderbroadcastsContextChangedMessagewith an immutable snapshot of the new context value, allowing features to respond on the messaging threadFeaturenow provides a virtualMakeMessageHandlermethod, optionally returning astd::functionweak_ptrto itselfCore:MessageBusMakeMessageHandleron all featuresMessageBusinto theCoreContextProviderCoretears down messaging state