refactor: fix thread safety and remove dead code#8
Open
avelytchko wants to merge 2 commits intomainfrom
Open
Conversation
Critical fixes: - Add mutex protection for m_session_key to prevent race conditions - Replace C-style cast with static_cast in write_callback() Dead code removal: - Remove unused sync method LastfmApi::authenticate() - Remove unused sync method LastfmApi::update_now_playing() - Remove unused async method LastfmApi::scrobble_track_async() Thread safety improvements: - Protect all m_session_key access with std::mutex - Fix concurrent access from main thread, network thread, and async callbacks - Add lock guards to 10 methods accessing shared session state Code quality: - Removed 93 lines of dead code - Eliminated potential race conditions - Improved type safety with static_cast Kept methods still in use: - LastfmApi::get_auth_url() - used in Mac preferences UI - LastfmApi::authenticate_async() - used in Mac preferences UI - LastfmApi::scrobble_track() - used by queue processor - LastfmApi::update_now_playing_async() - used by playback callback
📝 WalkthroughWalkthroughThis refactoring commit enhances thread safety in the Last.fm API integration by adding mutex protection for session key access. It removes unused synchronous methods (authenticate, update_now_playing, scrobble_track_async) and introduces async variants with callbacks, modernizes casts to static_cast, and eliminates dead code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
foobar2000/foo_mac_scrobble/lastfm_api.cpp (3)
135-182: Same double-lock pattern asscrobble_track.Similar consolidation opportunity exists here - the early check (lines 137-141) and param building (lines 143-147) could share one lock acquisition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@foobar2000/foo_mac_scrobble/lastfm_api.cpp` around lines 135 - 182, LastfmApi::validate_session currently acquires m_session_mutex twice (once to check m_session_key empty and again to read m_api_key/m_session_key into params); merge these into a single critical section to avoid redundant locking: acquire std::lock_guard<std::mutex> lock(m_session_mutex) once at the start of the function, perform the emptiness check and populate params = {{"method","auth.getSessionInfo"},{"api_key",m_api_key},{"sk",m_session_key}} while holding the lock, then release it before making the network call (keeping subsequent locks where you mutate m_session_key or read its length as needed).
106-118: Consider combining lock acquisitions to eliminate TOCTOU window.The current pattern acquires the lock twice: once in
is_authenticated()(line 108) and again when building params (line 113). Between these acquisitions, another thread could clearm_session_key. While this is safe (the request would simply fail), a single lock acquisition would be more efficient and eliminate the race window.Proposed refactor
bool LastfmApi::scrobble_track(const TrackInfo& track) { - if (!is_authenticated()) - return false; - std::map<std::string, std::string> params; { std::lock_guard<std::mutex> lock(m_session_mutex); + if (m_session_key.empty()) + return false; params = { {"method", "track.scrobble"}, {"api_key", m_api_key}, {"sk", m_session_key}, {"artist", track.artist}, {"track", track.track}, {"timestamp", std::to_string(track.timestamp)}, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@foobar2000/foo_mac_scrobble/lastfm_api.cpp` around lines 106 - 118, The scrobble_track function currently calls is_authenticated() (which locks m_session_mutex) and then reacquires m_session_mutex to build params, leaving a TOCTOU window; fix this by acquiring m_session_mutex once at the start of LastfmApi::scrobble_track, perform the authentication check (inspect m_session_key/m_api_key directly instead of calling is_authenticated()), and then build the params while still holding that same lock; reference m_session_mutex, m_session_key, m_api_key, is_authenticated(), and LastfmApi::scrobble_track when making the change.
524-537: Same double-lock pattern applies here.The
is_authenticated()call and the subsequent lock acquisition forparams["sk"]could be combined into a single critical section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@foobar2000/foo_mac_scrobble/lastfm_api.cpp` around lines 524 - 537, In update_now_playing_async, avoid the double-lock pattern by performing the authenticated check and reading m_session_key under the same m_session_mutex lock: acquire std::lock_guard<std::mutex> lock(m_session_mutex) first, call is_authenticated() (or check the underlying authenticated state used by is_authenticated()), and if authenticated set params["sk"] = m_session_key; if not, return; this ensures the is_authenticated() check and access to m_session_key happen atomically without separate unsynchronized and synchronized regions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@foobar2000/foo_mac_scrobble/lastfm_api.h`:
- Around line 13-14: The header lastfm_api.h is missing the <functional> include
needed for the std::function type used by authenticate_async; add `#include`
<functional> to the top of lastfm_api.h so the declaration of
authenticate_async(std::function<void(bool success)>) compiles reliably across
toolchains.
---
Nitpick comments:
In `@foobar2000/foo_mac_scrobble/lastfm_api.cpp`:
- Around line 135-182: LastfmApi::validate_session currently acquires
m_session_mutex twice (once to check m_session_key empty and again to read
m_api_key/m_session_key into params); merge these into a single critical section
to avoid redundant locking: acquire std::lock_guard<std::mutex>
lock(m_session_mutex) once at the start of the function, perform the emptiness
check and populate params =
{{"method","auth.getSessionInfo"},{"api_key",m_api_key},{"sk",m_session_key}}
while holding the lock, then release it before making the network call (keeping
subsequent locks where you mutate m_session_key or read its length as needed).
- Around line 106-118: The scrobble_track function currently calls
is_authenticated() (which locks m_session_mutex) and then reacquires
m_session_mutex to build params, leaving a TOCTOU window; fix this by acquiring
m_session_mutex once at the start of LastfmApi::scrobble_track, perform the
authentication check (inspect m_session_key/m_api_key directly instead of
calling is_authenticated()), and then build the params while still holding that
same lock; reference m_session_mutex, m_session_key, m_api_key,
is_authenticated(), and LastfmApi::scrobble_track when making the change.
- Around line 524-537: In update_now_playing_async, avoid the double-lock
pattern by performing the authenticated check and reading m_session_key under
the same m_session_mutex lock: acquire std::lock_guard<std::mutex>
lock(m_session_mutex) first, call is_authenticated() (or check the underlying
authenticated state used by is_authenticated()), and if authenticated set
params["sk"] = m_session_key; if not, return; this ensures the
is_authenticated() check and access to m_session_key happen atomically without
separate unsynchronized and synchronized regions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a962bf21-f2a4-4ff5-b819-cd4c6426b2c0
📒 Files selected for processing (3)
COMMIT_MESSAGE.txtfoobar2000/foo_mac_scrobble/lastfm_api.cppfoobar2000/foo_mac_scrobble/lastfm_api.h
Comment on lines
+13
to
14
| #include <mutex> | ||
| #include <string> |
There was a problem hiding this comment.
Missing <functional> header for std::function usage.
Line 63 declares authenticate_async with a std::function<void(bool success)> parameter, but <functional> is not included. While some standard library implementations may transitively include it, this can cause compilation failures on stricter or different compiler configurations.
Proposed fix
`#include` <mutex>
+#include <functional>
`#include` <string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #include <mutex> | |
| #include <string> | |
| `#include` <mutex> | |
| `#include` <functional> | |
| `#include` <string> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@foobar2000/foo_mac_scrobble/lastfm_api.h` around lines 13 - 14, The header
lastfm_api.h is missing the <functional> include needed for the std::function
type used by authenticate_async; add `#include` <functional> to the top of
lastfm_api.h so the declaration of authenticate_async(std::function<void(bool
success)>) compiles reliably across toolchains.
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
1 participant
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.
refactor: fix thread safety and remove dead code
Critical fixes:
Dead code removal:
Thread safety improvements:
Code quality:
Kept methods still in use:
Summary by CodeRabbit
Release Notes
New Features
Refactor