Refactor mutex locking to use std::lock_guard, std::unique_lock#3050
Refactor mutex locking to use std::lock_guard, std::unique_lock#3050salix5 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors multiple UI/networking code paths to use RAII-based mutex management (std::lock_guard / std::unique_lock) instead of manually calling lock()/unlock() on std::mutex, reducing the risk of leaks on early returns and improving exception-safety.
Changes:
- Replaced many
gMutex.lock()/gMutex.unlock()pairs withstd::lock_guard<std::mutex>scoped locking. - Introduced
std::unique_lock<std::mutex>where early unlock is required (e.g., replay save flows). - Added additional scoping blocks to ensure locks are released before waits/signals.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gframe/single_mode.cpp | Converts several UI-state updates and replay-save flows to lock_guard/unique_lock. |
| gframe/replay_mode.cpp | Converts parts of EndDuel/ReplayAnalyze to RAII locking, but still contains direct gMutex.lock()/unlock() elsewhere. |
| gframe/menu_handler.cpp | Replaces manual mutex lock/unlock around message box / popup UI interactions. |
| gframe/game.cpp | Uses lock_guard for resize/draw critical sections instead of manual locking. |
| gframe/event_handler.cpp | Replaces manual locking for scroll-driven UI refresh sections. |
| gframe/duelclient.cpp | Large-scale replacement of manual locks with lock_guard, plus unique_lock where early unlock is needed. |
| gframe/deck_con.cpp | Replaces manual locking around deck-builder UI popups and big-card display with lock_guard. |
| gframe/client_field.cpp | Converts select-option popup locking to lock_guard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(is_swaping) { | ||
| mainGame->gMutex.lock(); | ||
| std::lock_guard<std::mutex> lock(mainGame->gMutex); | ||
| mainGame->dField.ReplaySwap(); | ||
| mainGame->gMutex.unlock(); | ||
| is_swaping = false; | ||
| } |
There was a problem hiding this comment.
This file still directly calls mainGame->gMutex.lock() / unlock() for replay-skipping (e.g., in ReplayThread() and in ReplayAnalyze() when clearing isReplaySkiping). That undermines the PR goal of not locking/unlocking a std::mutex directly. Consider refactoring the replay-skip lock into a long-lived std::unique_lock<std::mutex> owned by ReplayMode (e.g., std::optional<std::unique_lock<std::mutex>>) so you can acquire/release it without touching gMutex.lock()/unlock() and avoid cross-function lock/unlock coupling.
There was a problem hiding this comment.
Humans call it historical issues.
|
https://github.com/mercury233/ygopro/tree/patch-network-queue It is better to make all network event handled in the main thread. I'm working on this. |
|
你的意思是: |
|
目前master的写法是需要的,而且还有一些遗漏。如果按通用做法,在网络线程把消息放入队列,在主线程处理,就可以去掉大部分mutex。 |
std::mutexshould not be locked or unlocked directly.It is recommended to use
std::lock_guardorstd::unique_lockto manage the mutex.@Wind2009-Louse @fallenstardust