🐛 fix: Remove unnecessary mutex locks and improve error handling#1974
🐛 fix: Remove unnecessary mutex locks and improve error handling#1974
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve error handling for Discord API calls and remove mutex locks that were deemed unnecessary. However, the mutex lock removals introduce potential race conditions that need to be addressed.
- Improved error handling for
FollowupMessageCreateandChannelMessageEditComplexcalls by capturing and logging errors - Removed mutex locks around
LastSaveTimefield assignments in the save data flow - Changed Boost Menu visibility to always show regardless of contract state
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/boost/contract.go | Added error handling for Discord API followup message creation and message edit operations, now properly capturing and logging errors instead of discarding them |
| src/boost/boost_datastore.go | Removed mutex locks protecting LastSaveTime assignments during contract save operations |
| src/boost/boost_button_reactions.go | Removed conditional check that previously hid the Boost Menu during signup state, menu now always displays |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, c := range Contracts { | ||
| saveSqliteData(c) | ||
| c.mutex.Lock() | ||
| c.LastSaveTime = time.Now() |
There was a problem hiding this comment.
Removing the mutex lock around LastSaveTime assignment creates a potential race condition. This field is accessed without locking in the loop that iterates over all contracts. When multiple goroutines access different contracts or when one goroutine reads while another writes to the same contract's LastSaveTime, data races can occur. The Contract struct has a mutex field for thread safety, and this shared state modification should be protected.
| minValues := 0 | ||
| out = append(out, discordgo.ActionsRow{ | ||
| Components: []discordgo.MessageComponent{ | ||
| discordgo.SelectMenu{ | ||
| CustomID: "menu#" + contract.ContractHash, | ||
| Placeholder: "Boost Menu", | ||
| MinValues: &minValues, | ||
| MaxValues: 1, | ||
| Options: menuOptions, | ||
| }, | ||
| }) | ||
| } | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Removing the conditional check if contract.State != ContractStateSignup changes the behavior to always show the Boost Menu, even during the signup phase. Previously, the menu was hidden during signup state. This appears to be an intentional behavior change, but if the menu contains actions that should not be available during signup (like the token-related menu options conditionally added on lines 486-537), this could lead to users accessing functionality in an inappropriate state. Consider whether all menu options are appropriate for the signup phase, or if certain options should be conditionally excluded based on contract state.
|
|
||
| saveSqliteData(contract) | ||
| contract.mutex.Lock() | ||
| contract.LastSaveTime = time.Now() |
There was a problem hiding this comment.
Removing the mutex lock around LastSaveTime assignment creates a potential race condition. The LastSaveTime field is read without locking on lines 78 and 83 (via time.Since(contract.LastSaveTime)), and written without locking on line 90. This can lead to data races when multiple goroutines call saveData concurrently or when one goroutine reads while another writes. The Contract struct has a mutex field specifically for thread safety, and since LastSaveTime is part of the shared state, its access should be protected by the mutex.
No description provided.