TQ: Add NodeCtx for use in Node API
#8629
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rather than passing in separate parameters to `Node` methods, we pass in a context shared with the caller. Importantly, due to the use of wrapper traits, the caller and methods are only able to mutate things that they each should be able to mutate. This makes the function signatures shorter and also eliminates the case where we end up returning an `Option<PersistentState>`. This was getting tedious, not just here, but in other code not yet pushed. Since the caller already has to check if there are any outgoing messages to send after each api call, they can also check to see if the persistent state has changed. This isn't much of a burden and simplifies the `Node` internals somewhat. It also eliminates a clone. We can add other state to the context as needed over time.
andrewjstone
commented
Jul 17, 2025
| @@ -147,7 +148,8 @@ impl CoordinatorState { | |||
| // | |||
| // This method is "in progress" - allow unused parameters for now | |||
| #[expect(unused)] | |||
| pub fn send_msgs(&mut self, now: Instant, outbox: &mut Vec<Envelope>) { | |||
| pub fn send_msgs(&mut self, ctx: &mut impl NodeHandlerCtx) { | |||
There was a problem hiding this comment.
The NodeHandlerCtx trait is not dyn safe (object safe) due to the generic parameter on update_persistent_state. Therefore we use a trait impl parameter. This also avoids dynamic dispatch, potentially at the cost of monomorphization during compilation.
jgallagher
approved these changes
Jul 18, 2025
Thanks @jgallagher. Yes, this is barely related to the TQ protocol itself and I really did just want a sanity check. It's just to make the API a touch easier to use (and build). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rather than passing in separate parameters to
Nodemethods, we pass in a context shared with the caller. Importantly, due to the use of wrapper traits, the caller and methods are only able to mutate things that they each should be able to mutate.This makes the function signatures shorter and also eliminates the case where we end up returning an
Option<PersistentState>. This was getting tedious, not just here, but in other code not yet pushed. Since the caller already has to check if there are any outgoing messages to send after each api call, they can also check to see if the persistent state has changed. This isn't much of a burden and simplifies theNodeinternals somewhat. It also eliminates a clone. We can add other state to the context as needed over time.