Fix crashes on saving scores with parts#33618
Open
miiizen wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR advances the muse submodule and makes several concurrency-related changes: EID::newUnique() uses thread-local RNG and distribution; EIDRegister adds a mutable std::shared_mutex and takes a std::lock_guard in registerItemEID; Writer replaces GlobalInject with GlobalThreadSafeInject for IApplication and runs masterScore()->checkMidiMapping() only when score->isMaster(). 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engraving/infrastructure/eidregister.cpp`:
- Line 43: The methods accessing the shared maps are inconsistently
synchronized: add appropriate locking around all accesses to m_itemToEid and
m_eidToItem—use an exclusive lock (std::lock_guard or std::unique_lock on
m_mutex) at the start of removeItem (since it modifies both maps) and use shared
locks (std::shared_lock on m_mutex) at the start of itemFromEID and EIDFromItem
(since they only read); ensure the locks are acquired before any map access in
the functions registerItemEID, removeItem, itemFromEID, and EIDFromItem to
prevent data races.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 858bb581-22f9-43bc-aca7-4cb8a773a10c
📒 Files selected for processing (6)
musesrc/engraving/infrastructure/eid.cppsrc/engraving/infrastructure/eidregister.cppsrc/engraving/infrastructure/eidregister.hsrc/engraving/rw/write/writer.cppsrc/engraving/rw/write/writer.h
| IF_ASSERT_FAILED(eid.isValid() && item) { | ||
| return; | ||
| } | ||
| std::lock_guard lock(m_mutex); |
Contributor
There was a problem hiding this comment.
Incomplete synchronization: other methods also need locks.
While registerItemEID now locks m_mutex, the other methods (removeItem, itemFromEID, EIDFromItem) that access the shared maps remain unsynchronized. Concurrent access from multiple threads can still cause data races and corruption.
removeItem(lines 60-78) modifies both maps and needs an exclusive lock (std::lock_guardorstd::unique_lock)itemFromEID(lines 80-87) readsm_eidToItemand needs a shared lock (std::shared_lock)EIDFromItem(lines 89-93) readsm_itemToEidand needs a shared lock (std::shared_lock)
🔒 Proposed fix to add synchronization to all methods
Add to removeItem:
void EIDRegister::removeItem(const EngravingObject* item)
{
+ std::lock_guard lock(m_mutex);
// NOTE: needed only when elements are removed during read (e.g. broken spanners)Add to itemFromEID:
EngravingObject* EIDRegister::itemFromEID(const EID& eid) const
{
+ std::shared_lock lock(m_mutex);
auto iter = m_eidToItem.find(eid);Add to EIDFromItem:
EID EIDRegister::EIDFromItem(const EngravingObject* item) const
{
+ std::shared_lock lock(m_mutex);
auto iter = m_itemToEid.find(const_cast<EngravingObject*>(item));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/engraving/infrastructure/eidregister.cpp` at line 43, The methods
accessing the shared maps are inconsistently synchronized: add appropriate
locking around all accesses to m_itemToEid and m_eidToItem—use an exclusive lock
(std::lock_guard or std::unique_lock on m_mutex) at the start of removeItem
(since it modifies both maps) and use shared locks (std::shared_lock on m_mutex)
at the start of itemFromEID and EIDFromItem (since they only read); ensure the
locks are acquired before any map access in the functions registerItemEID,
removeItem, itemFromEID, and EIDFromItem to prevent data races.
miiizen
force-pushed
the
33516-partSaveCrash
branch
from
ec4ac8c to
acc3f9a
Compare
Contributor
|
Actionable comments posted: 0 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Resolves: #33516
This PR makes EIDRegister and EID::newUnique thread safe, as during saving we create and register new EIDs.
I have also made sure
MasterScore::checkMidiMappingis only called for the master score. There's no need to repeat this for each part.The crashes were introduced in #32702