fix(hgraph): fix concurrency issues during hgraph insert (#1431) #1496
Conversation
Signed-off-by: jinjiabao.jjb <[email protected]>
Summary of ChangesHello @inabao, 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 and resolves a concurrency bug within the "HGraph" component, specifically during the insertion of new points. By implementing proper locking mechanisms around critical data modifications, the change ensures data integrity and prevents race conditions that could arise from simultaneous write operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 aims to fix a concurrency issue in HGraph::add_one_point by adding a std::shared_lock around vector insertion calls. While this change correctly prevents a race condition between vector insertion and resizing operations, it is part of a function with a pre-existing and more critical potential for deadlocks due to its locking strategy. The function acquires a std::unique_lock after this new block, which can lead to deadlocks if called from a context that already holds a lock on the same mutex. This is a critical issue that needs to be addressed.
| { | ||
| std::shared_lock add_lock(add_mutex_); | ||
| this->basic_flatten_codes_->InsertVector(data, inner_id); | ||
| if (use_reorder_) { | ||
| this->high_precise_codes_->InsertVector(data, inner_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
While adding this shared_lock correctly fixes a race condition with resize, the overall locking strategy in add_one_point is unsafe and can lead to deadlocks.
Immediately after this block, a std::unique_lock is acquired on the same add_mutex_ (line 1013). This shared_lock -> unique_lock sequence on a non-recursive mutex is dangerous:
- Deadlock on re-entry: If a caller already holds a
unique_lockonadd_mutex_(e.g., inHGraph::Merge) and callsadd_one_point, it will deadlock when thisshared_lockis acquired. - Deadlock on indirect recursive locking: The
unique_lockon line 1013 is held during calls to other functions likegraph_add_one. If any function in that call path (e.g.,resize) also tries to acquire aunique_lockonadd_mutex_, it will deadlock.
This indicates a larger design issue with locking in this part of the code. The function add_one_point should likely not manage locks itself. Instead, it could be refactored into unlocked helper methods, with the public API methods (Add, Merge, etc.) being responsible for acquiring locks once at a higher level.
Signed-off-by: jinjiabao.jjb <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## 0.15 #1496 +/- ##
==========================================
+ Coverage 90.23% 90.34% +0.10%
==========================================
Files 221 218 -3
Lines 15097 13499 -1598
==========================================
- Hits 13623 12195 -1428
+ Misses 1474 1304 -170
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
cherry-pick: #1425, #1357 Signed-off-by: Xiangyu Wang <[email protected]>
inabao
requested review from
LHT129,
ShawnShawnYou and
jiaweizone
as code owners
Signed-off-by: jinjiabao.jjb <[email protected]>
Signed-off-by: jinjiabao.jjb <[email protected]>
cp #1431 to 0.15