fix(rdmacm): use or_insert_with() to avoid eager DeviceContext creation
#87
Conversation
Summary of ChangesHello @dragonJACson, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a subtle but critical bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
…eation `or_insert()` eagerly evaluates the default argument even when the entry exists, causing a new `DeviceContext` to be created and immediately dropped. This triggers `ibv_close_device()` on the shared verbs context, corrupting the cached entry. Switch to `or_insert_with()` so the `DeviceContext` is only constructed when the `HashMap` entry is missing. Signed-off-by: Luke Yue <lukedyue@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical bug where or_insert() was causing eager creation and destruction of DeviceContext, leading to corruption of the shared verbs context. The switch to or_insert_with() is the correct approach to ensure the DeviceContext is only created when necessary. The addition of the test_get_device_context_caches_correctly test is excellent, as it properly verifies the fix and prevents regressions. I have a couple of suggestions to improve the code's readability and efficiency.
| let device_ctx = guard.entry((*cm_id.as_ptr()).verbs as usize).or_insert_with(|| { | ||
| Arc::new(DeviceContext { | ||
| // Safe due to the is_null() check above. | ||
| context: NonNull::new((*cm_id.as_ptr()).verbs).unwrap(), | ||
| })); | ||
| }) | ||
| }); |
There was a problem hiding this comment.
For better readability and to adhere to common Rust formatting conventions, it's recommended to format this chained method call across multiple lines. This makes the code easier to parse visually, especially with the closure.
| let device_ctx = guard.entry((*cm_id.as_ptr()).verbs as usize).or_insert_with(|| { | |
| Arc::new(DeviceContext { | |
| // Safe due to the is_null() check above. | |
| context: NonNull::new((*cm_id.as_ptr()).verbs).unwrap(), | |
| })); | |
| }) | |
| }); | |
| let device_ctx = guard | |
| .entry((*cm_id.as_ptr()).verbs as usize) | |
| .or_insert_with(|| { | |
| Arc::new(DeviceContext { | |
| // Safe due to the is_null() check above. | |
| context: NonNull::new((*cm_id.as_ptr()).verbs).unwrap(), | |
| }) | |
| }); |
| assert!( | ||
| Arc::ptr_eq(&ctx1.clone().unwrap(), &ctx2.clone().unwrap()), | ||
| "ctx1 and ctx2 should point to the same DeviceContext" | ||
| ); | ||
| assert!( | ||
| Arc::ptr_eq(&ctx2.clone().unwrap(), &ctx3.clone().unwrap()), | ||
| "ctx2 and ctx3 should point to the same DeviceContext" | ||
| ); |
There was a problem hiding this comment.
To make the code more idiomatic and efficient, you can use as_ref() instead of clone() when comparing the Arc pointers. as_ref() borrows the content of the Option without incrementing the Arc's reference count, which is more efficient as you only need a reference for Arc::ptr_eq.
| assert!( | |
| Arc::ptr_eq(&ctx1.clone().unwrap(), &ctx2.clone().unwrap()), | |
| "ctx1 and ctx2 should point to the same DeviceContext" | |
| ); | |
| assert!( | |
| Arc::ptr_eq(&ctx2.clone().unwrap(), &ctx3.clone().unwrap()), | |
| "ctx2 and ctx3 should point to the same DeviceContext" | |
| ); | |
| assert!( | |
| Arc::ptr_eq(ctx1.as_ref().unwrap(), ctx2.as_ref().unwrap()), | |
| "ctx1 and ctx2 should point to the same DeviceContext" | |
| ); | |
| assert!( | |
| Arc::ptr_eq(ctx2.as_ref().unwrap(), ctx3.as_ref().unwrap()), | |
| "ctx2 and ctx3 should point to the same DeviceContext" | |
| ); |
dragonJACson
force-pushed
the
fix/cm-device
branch
from
4defef9 to
73ea8e1
Compare
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
or_insert()eagerly evaluates the default argument even when the entry exists, causing a newDeviceContextto be created and immediately dropped. This triggersibv_close_device()on the shared verbs context, corrupting the cached entry.Switch to
or_insert_with()so theDeviceContextis only constructed when theHashMapentry is missing.