Convert monitor update macros to ChannelManager methods #4291
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
joostjager
force-pushed
the
update-handling-refactor
branch
from
dd25afd to
f687020
Compare
joostjager
changed the title
Pure code move except for the context logger which is now instantiated when needed in handle_monitor_update_internal.
joostjager
force-pushed
the
update-handling-refactor
branch
from
f687020 to
dfbf531
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4291 +/- ##
==========================================
- Coverage 89.38% 86.65% -2.73%
==========================================
Files 180 158 -22
Lines 139834 102332 -37502
Branches 139834 102332 -37502
==========================================
- Hits 124985 88674 -36311
+ Misses 12262 11246 -1016
+ Partials 2587 2412 -175
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
I'm kind of meh on whether this improves readability, tbh. Is it just the CM argument?
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
IMO this is a readability reduction - instead of having the code next to the macros that call it (that its effectively an implementation detail of), this moves the code to some unrelated part of the file. If we can make the macros a function great, but separating the code from the code calling it seems like a loss.
|
In my opinion those functions really are methods based on what they access inside But as you both don't share this preference, the code should probably remain what it is. |
I agree with this, but that really applies to the things that are still macros after this PR - the things that are being called should be methods on |
|
Done in the latest push. This does introduce some duplication with the lock dropping and completion handling repeated at each call site. However, I think this is a worthwhile trade-off. As someone relatively new to the codebase, the macros have been a constant source of confusion. They obscure control flow, don't show up in symbol navigation, and create gaps in the call stack during debugging. The explicit code is much easier to follow and reason about, in my opinion. Let's decide if we want to go this way (I think we should), and then I'll do a closer inspection of the claude generated code to see if it's all sound. |
joostjager
force-pushed
the
update-handling-refactor
branch
from
a12908f to
8cd1f51
Compare
|
Concept ACK, needs a fair amount of cleanup and would prefer if the last commit were broken up a bunch though |
| &mut peer_state.in_flight_monitor_updates, | ||
| &mut peer_state.monitor_update_blocked_actions, | ||
| &mut peer_state.pending_msg_events, | ||
| peer_state.is_connected, |
There was a problem hiding this comment.
Is it possible to just pass in &mut peer_state instead to DRY things?
There was a problem hiding this comment.
It leads to borrow checker problems.
|
|
||
| /// Like [`Self::handle_new_monitor_update`], but also returns whether this specific update | ||
| /// completed (as opposed to being in-progress). | ||
| fn handle_new_monitor_update_with_status( |
There was a problem hiding this comment.
This is only called in one place, seems we can get rid of it
There was a problem hiding this comment.
It's called in two places
joostjager
force-pushed
the
update-handling-refactor
branch
5 times, most recently
from
89a8981 to
f59d1ab
Compare
|
Broke up the big refactor commit and scrutinized the claude code. I think it should be much more reviewable now. |
joostjager
changed the title
Extract the monitor update completion logic from the handle_monitor_update_completion macro into two new helper methods: - prepare_monitor_update_completion_data: Collects all necessary data from the channel while peer_state locks are still held - handle_monitor_update_completion_data: Processes the collected data after locks have been released This refactoring improves code organization by separating the data extraction phase (which requires locks) from the processing phase (which should happen after locks are released). The macro is now a thin wrapper that calls these two methods with proper lock management. The new MonitorUpdateCompletionData struct serves as a container for all the data that needs to be passed between the two phases.
Expand the macro at its three call sites and remove the macro definition, as it no longer provides significant abstraction benefit after refactoring the core logic into helper methods. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Convert the handle_initial_monitor! macro to a method that returns optional completion data, allowing callers to release locks before processing the completion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Convert the handle_post_close_monitor_update! macro to a method that returns optional completion actions, allowing callers to release locks before processing the completion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Convert the handle_new_monitor_update macro to methods and update all call sites. Adds handle_new_monitor_update and handle_new_monitor_update_with_status methods that return completion data for processing after locks are released. Also removes handle_monitor_update_completion macro as it's no longer needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
joostjager
force-pushed
the
update-handling-refactor
branch
from
f59d1ab to
abdbf82
Compare
4 participants
Replaces five monitor update handling macros with proper methods on
ChannelManager:handle_monitor_update_completion!handle_initial_monitor!handle_post_close_monitor_update!handle_new_monitor_update_locked_actions_handled_by_caller!handle_new_monitor_update!Introduces
MonitorUpdateCompletionDataenum to cleanly separate the data extraction phase (requires locks) from the processing phase (after locks released).Motivation
Macros make this code hard to debug, navigate, and understand due to implicit variable capture and non-obvious control flow. Methods provide clear signatures, explicit parameters, and proper stack traces.
No behavioral changes.