[ISSUE #3517]⚡️Memory Management Fix for AtomicPtr-based Address Fields in HA Client#3518
[ISSUE #3517]⚡️Memory Management Fix for AtomicPtr-based Address Fields in HA Client#3518rocketmq-rust-bot merged 1 commit intomainfrom
Conversation
…ds in HA Client
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
added
AI review first
WalkthroughThe changes replace asynchronous locking with atomic pointer operations for managing HA master addresses in the HA client and related service layers. They implement previously missing trait methods, ensure safe memory management by freeing old pointers, and delegate address updates through the appropriate service and client structures. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as DefaultHAService
participant Client as DefaultHAClient
participant Memory as System Memory
Service->>Client: update_ha_master_address(new_addr)
Client->>Memory: Free old pointer (if any)
Client->>Memory: Allocate and store new pointer
sequenceDiagram
participant General as GeneralHAClient
participant Default as DefaultHAClient
participant Auto as AutoSwitchHAClient
alt default_ha_client is set
General->>Default: update_ha_master_address(new_addr)
else auto_switch_ha_client is set
General->>Auto: update_ha_master_address(new_addr)
else no client set
General->>General: panic("No HA service is set")
end
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR replaces RwLock<Option<String>> with AtomicPtr<String> for address fields in DefaultHAClient to improve memory management and implements the previously todo!() methods.
- Implement
update_ha_master_addressinGeneralHAClient - Forward
update_ha_master_addressinDefaultHAService - Swap out lock-based fields for
AtomicPtrinDefaultHAClientand handle pointer allocation/free
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| general_ha_client.rs | Added conditional logic to forward update_ha_master_address |
| default_ha_service.rs | Replaced todo!() with a forward to inner HA client’s method |
| default_ha_client.rs | Swapped RwLock for AtomicPtr, updated initialization, getters, and setter with raw pointers |
Comments suppressed due to low confidence (2)
rocketmq-store/src/ha/default_ha_client.rs:145
- The new AtomicPtr-based getters and setters lack unit tests. Please add tests covering null-pointer behavior, address updates, and concurrent access to validate correct pointer management.
let addr_ptr = self.master_ha_address.load(Ordering::SeqCst);
| fn update_ha_master_address(&self, new_address: &str) { | ||
| todo!() | ||
| // Safely free the old pointer before storing the new one | ||
| let old_address = self.master_address.load(Ordering::SeqCst); |
There was a problem hiding this comment.
Loading the old pointer and freeing it before storing the new one can introduce race conditions under concurrent access. Use swap to atomically exchange the pointer (let old_ptr = self.master_address.swap(new_ptr, Ordering::SeqCst)) and then free old_ptr.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rocketmq-store/src/ha/default_ha_client.rs(4 hunks)rocketmq-store/src/ha/default_ha_service.rs(1 hunks)rocketmq-store/src/ha/general_ha_client.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: auto-approve
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (5)
rocketmq-store/src/ha/default_ha_service.rs (1)
202-202: LGTM! Proper delegation implementation.The implementation correctly delegates to the underlying HA client, following the same pattern as
update_master_address. This completes the service layer implementation for HA master address updates.rocketmq-store/src/ha/general_ha_client.rs (1)
84-90: LGTM! Consistent delegation pattern implementation.The implementation correctly follows the established pattern used by
update_master_addressand other methods in this file. The delegation logic and error handling are appropriate and consistent.rocketmq-store/src/ha/default_ha_client.rs (3)
66-66: LGTM! Performance improvement with atomic operations.The change from
Arc<RwLock<Option<String>>>toArc<AtomicPtr<String>>eliminates async locking overhead and provides better performance for address management in the HA client.
123-123: LGTM! Constructor updated appropriately.The constructor initialization is correctly updated to use
AtomicPtr::default()for the new field type.
144-152: LGTM! Atomic operations implementation is correct.The
get_ha_master_addressmethod correctly uses atomic load operations and safe pointer dereferencing to retrieve the address without async locking.
| fn update_ha_master_address(&self, new_address: &str) { | ||
| todo!() | ||
| // Safely free the old pointer before storing the new one | ||
| let old_address = self.master_address.load(Ordering::SeqCst); | ||
| if !old_address.is_null() { | ||
| unsafe { | ||
| // Convert the old pointer back into a Box to free the memory | ||
| let _ = Box::from_raw(old_address); | ||
| } | ||
| } | ||
| // Store the new address | ||
| let new_address_ptr = Box::into_raw(Box::new(new_address.to_string())); | ||
| self.master_address.store(new_address_ptr, Ordering::SeqCst); | ||
| } |
There was a problem hiding this comment.
Critical bug: Method is updating the wrong field!
The update_ha_master_address method is incorrectly updating self.master_address instead of self.master_ha_address. This appears to be a copy-paste error from the update_master_address method.
Apply this fix:
fn update_ha_master_address(&self, new_address: &str) {
// Safely free the old pointer before storing the new one
- let old_address = self.master_address.load(Ordering::SeqCst);
+ let old_address = self.master_ha_address.load(Ordering::SeqCst);
if !old_address.is_null() {
unsafe {
// Convert the old pointer back into a Box to free the memory
let _ = Box::from_raw(old_address);
}
}
// Store the new address
let new_address_ptr = Box::into_raw(Box::new(new_address.to_string()));
- self.master_address.store(new_address_ptr, Ordering::SeqCst);
+ self.master_ha_address.store(new_address_ptr, Ordering::SeqCst);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn update_ha_master_address(&self, new_address: &str) { | |
| todo!() | |
| // Safely free the old pointer before storing the new one | |
| let old_address = self.master_address.load(Ordering::SeqCst); | |
| if !old_address.is_null() { | |
| unsafe { | |
| // Convert the old pointer back into a Box to free the memory | |
| let _ = Box::from_raw(old_address); | |
| } | |
| } | |
| // Store the new address | |
| let new_address_ptr = Box::into_raw(Box::new(new_address.to_string())); | |
| self.master_address.store(new_address_ptr, Ordering::SeqCst); | |
| } | |
| fn update_ha_master_address(&self, new_address: &str) { | |
| // Safely free the old pointer before storing the new one | |
| - let old_address = self.master_address.load(Ordering::SeqCst); | |
| + let old_address = self.master_ha_address.load(Ordering::SeqCst); | |
| if !old_address.is_null() { | |
| unsafe { | |
| // Convert the old pointer back into a Box to free the memory | |
| let _ = Box::from_raw(old_address); | |
| } | |
| } | |
| // Store the new address | |
| let new_address_ptr = Box::into_raw(Box::new(new_address.to_string())); | |
| - self.master_address.store(new_address_ptr, Ordering::SeqCst); | |
| + self.master_ha_address.store(new_address_ptr, Ordering::SeqCst); | |
| } |
🤖 Prompt for AI Agents
In rocketmq-store/src/ha/default_ha_client.rs around lines 684 to 696, the
method update_ha_master_address is incorrectly updating the field
self.master_address instead of self.master_ha_address. To fix this, replace all
occurrences of self.master_address with self.master_ha_address in this method,
ensuring the old pointer is freed and the new address is stored in
self.master_ha_address.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3518 +/- ##
==========================================
- Coverage 26.18% 26.18% -0.01%
==========================================
Files 555 555
Lines 78635 78648 +13
==========================================
Hits 20593 20593
- Misses 58042 58055 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #3517
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor