From 4464ffb19ffe97332638619600326f2ce218d27b Mon Sep 17 00:00:00 2001 From: avelytchko <919635+avelytchko@users.noreply.github.com> Date: Thu, 5 Mar 2026 20:02:34 +0100 Subject: [PATCH 1/2] refactor: fix thread safety and remove dead code 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 --- foobar2000/foo_mac_scrobble/lastfm_api.cpp | 197 ++++++--------------- foobar2000/foo_mac_scrobble/lastfm_api.h | 23 +-- 2 files changed, 71 insertions(+), 149 deletions(-) diff --git a/foobar2000/foo_mac_scrobble/lastfm_api.cpp b/foobar2000/foo_mac_scrobble/lastfm_api.cpp index 8783231..d8aa2b6 100644 --- a/foobar2000/foo_mac_scrobble/lastfm_api.cpp +++ b/foobar2000/foo_mac_scrobble/lastfm_api.cpp @@ -67,6 +67,7 @@ void LastfmApi::set_credentials(const char* api_key, const char* api_secret) // Clear session if credentials change if (new_api_key != m_api_key || new_api_secret != m_api_secret) { + std::lock_guard lock(m_session_mutex); if (!m_session_key.empty() && foo_lastfm::cfg_debug_enabled.get()) { FB2K_console_formatter() << "Last.fm: Credentials changed - clearing session"; @@ -85,101 +86,21 @@ void LastfmApi::set_session_key(const char* session_key) new_session_key.erase(std::remove_if(new_session_key.begin(), new_session_key.end(), [](unsigned char c) { return std::isspace(c) || c == '\0'; }), new_session_key.end()); - if (new_session_key != m_session_key) { - m_session_key = new_session_key; - log_debug("Last.fm: Session key set (len=%d, value=%s)", (int)m_session_key.length(), - redact_secret(m_session_key).c_str()); - } -} - -std::string LastfmApi::get_auth_url() const -{ - // Generate URL for Last.fm authentication - return std::string(AUTH_URL) + m_api_key; -} - -bool LastfmApi::is_authenticated() const -{ - return !m_session_key.empty(); -} - -bool LastfmApi::authenticate(const std::string& token) -{ - log_debug("Last.fm: Starting authentication (token length: %d)", (int)token.length()); - std::map params{ - {"method", "auth.getSession"}, - {"api_key", m_api_key}, - {"token", token}, - }; - - std::string response; - if (!send_api_request(params, response)) - return false; - - // Parse authentication response - try - { - auto json_data = json::parse(response); - if (json_data.contains("error")) + std::lock_guard lock(m_session_mutex); + if (new_session_key != m_session_key) { - int code = json_data["error"].get(); - std::string msg = json_data["message"].get(); - FB2K_console_formatter() << "Last.fm ERROR: " << code << " - " << msg.c_str(); - return false; - } - - if (json_data.contains("session")) - { - auto session = json_data["session"]; - m_session_key = session.value("key", ""); - - // Save session to file - std::string username = session.value("name", ""); - if (foo_lastfm::g_session_manager) - { - foo_lastfm::g_session_manager->save_session(m_session_key, username); - } - - foo_lastfm::cfg_username.set(username.c_str()); - return true; + m_session_key = new_session_key; + log_debug("Last.fm: Session key set (len=%d, value=%s)", (int)m_session_key.length(), + redact_secret(m_session_key).c_str()); } - - FB2K_console_formatter() << "Last.fm ERROR: Unexpected JSON format (missing session)"; - return false; - } - catch (const std::exception& e) - { - FB2K_console_formatter() << "Last.fm JSON parse error: " << e.what(); - return false; } } -bool LastfmApi::update_now_playing(const TrackInfo& track) +bool LastfmApi::is_authenticated() const { - if (!is_authenticated()) - return false; - std::map params{ - {"method", "track.updateNowPlaying"}, - {"api_key", m_api_key}, - {"sk", m_session_key}, - {"artist", track.artist}, - {"track", track.track}, - }; - if (!track.album.empty()) - params["album"] = track.album; - if (!track.album_artist.empty()) - params["albumArtist"] = track.album_artist; - if (track.duration > 0) - params["duration"] = std::to_string(track.duration); - if (track.track_number > 0) - params["trackNumber"] = std::to_string(track.track_number); - - std::string response; - bool ok = send_api_request(params, response); - if (!ok) - FB2K_console_formatter() << "Last.fm: Failed to update now playing"; - return ok; + std::lock_guard lock(m_session_mutex); + return !m_session_key.empty(); } bool LastfmApi::scrobble_track(const TrackInfo& track) @@ -187,10 +108,14 @@ bool LastfmApi::scrobble_track(const TrackInfo& track) if (!is_authenticated()) return false; - std::map 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)}, - }; + std::map params; + { + std::lock_guard lock(m_session_mutex); + 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)}, + }; + } if (!track.album.empty()) params["album"] = track.album; if (!track.album_artist.empty()) @@ -209,11 +134,17 @@ bool LastfmApi::scrobble_track(const TrackInfo& track) bool LastfmApi::validate_session() { - if (m_session_key.empty()) - return false; + { + std::lock_guard lock(m_session_mutex); + if (m_session_key.empty()) + return false; + } - std::map params{ - {"method", "auth.getSessionInfo"}, {"api_key", m_api_key}, {"sk", m_session_key}}; + std::map params; + { + std::lock_guard lock(m_session_mutex); + params = {{"method", "auth.getSessionInfo"}, {"api_key", m_api_key}, {"sk", m_session_key}}; + } std::string response; CURLcode curl_res = CURLE_OK; @@ -232,13 +163,21 @@ bool LastfmApi::validate_session() if (!ok || http_code == 403 || http_code == 401) { FB2K_console_formatter() << "Last.fm: Saved session is invalid (HTTP " << http_code << "), clearing..."; - m_session_key.clear(); + { + std::lock_guard lock(m_session_mutex); + m_session_key.clear(); + } if (foo_lastfm::g_session_manager) foo_lastfm::g_session_manager->clear_session(); return false; } - FB2K_console_formatter() << "Last.fm: Session key validated (length: " << (int)m_session_key.length() << ")"; + int session_key_len; + { + std::lock_guard lock(m_session_mutex); + session_key_len = (int)m_session_key.length(); + } + FB2K_console_formatter() << "Last.fm: Session key validated (length: " << session_key_len << ")"; return true; } @@ -290,7 +229,7 @@ size_t LastfmApi::write_callback(void* c, size_t s, size_t n, void* u) { // CURL callback to collect response data size_t t = s * n; - ((std::string*)u)->append((char*)c, t); + static_cast(u)->append(static_cast(c), t); return t; } @@ -406,7 +345,10 @@ bool LastfmApi::send_api_request(const std::map& param { if (method != "auth.getSession" && method != "auth.getToken") { - m_session_key.clear(); + { + std::lock_guard lock(m_session_mutex); + m_session_key.clear(); + } foo_lastfm::cfg_session_key.set(""); if (foo_lastfm::cfg_debug_enabled.get()) { @@ -478,6 +420,12 @@ bool LastfmApi::send_api_request(const std::map& param return true; } +std::string LastfmApi::get_auth_url() const +{ + // Generate URL for Last.fm authentication + return std::string(AUTH_URL) + m_api_key; +} + void LastfmApi::execute_async_request(const std::map& params, std::function callback) { @@ -532,11 +480,15 @@ void LastfmApi::authenticate_async(const std::string& token, std::function lock(m_session_mutex); + m_session_key = session_key; + } if (foo_lastfm::g_session_manager) { - foo_lastfm::g_session_manager->save_session(m_session_key, username); + foo_lastfm::g_session_manager->save_session(session_key, username); } if (!username.empty()) { @@ -579,7 +531,10 @@ void LastfmApi::update_now_playing_async(const TrackInfo& track) std::map params; params["method"] = "track.updateNowPlaying"; params["api_key"] = m_api_key; - params["sk"] = m_session_key; + { + std::lock_guard lock(m_session_mutex); + params["sk"] = m_session_key; + } params["artist"] = track.artist; params["track"] = track.track; params["album"] = track.album; @@ -598,39 +553,3 @@ void LastfmApi::update_now_playing_async(const TrackInfo& track) (void)response; }); } - -void LastfmApi::scrobble_track_async(const TrackInfo& track) -{ - if (!is_authenticated() || track.artist.empty() || track.track.empty()) - { - return; - } - - std::map params; - params["method"] = "track.scrobble"; - params["api_key"] = m_api_key; - params["sk"] = m_session_key; - params["artist"] = track.artist; - params["track"] = track.track; - params["timestamp"] = std::to_string(track.timestamp); - params["album"] = track.album; - params["albumArtist"] = track.album_artist; - params["duration"] = std::to_string(track.duration); - if (track.track_number > 0) - { - params["trackNumber"] = std::to_string(track.track_number); - } - - execute_async_request(params, - [](bool success, const std::string& response) - { - if (!success && foo_lastfm::cfg_debug_enabled.get()) - { - FB2K_console_formatter() << "Last.fm: Scrobble failed silently in background"; - } - else if (success && foo_lastfm::cfg_debug_enabled.get()) - { - FB2K_console_formatter() << "Last.fm: Scrobble successful"; - } - }); -} diff --git a/foobar2000/foo_mac_scrobble/lastfm_api.h b/foobar2000/foo_mac_scrobble/lastfm_api.h index e742d82..ee86507 100644 --- a/foobar2000/foo_mac_scrobble/lastfm_api.h +++ b/foobar2000/foo_mac_scrobble/lastfm_api.h @@ -10,6 +10,7 @@ #include #include #include +#include #include class LastfmApi @@ -34,18 +35,15 @@ class LastfmApi TrackInfo() : duration(0), track_number(0), timestamp(0) {} }; - // Authenticates user with a token (synchronous) - bool authenticate(const std::string& token); - // Generates URL for user authentication - std::string get_auth_url() const; // Checks if the current session is authenticated bool is_authenticated() const; // Checks if a valid session exists - bool has_saved_session() const { return !m_session_key.empty(); } + bool has_saved_session() const { + std::lock_guard lock(m_session_mutex); + return !m_session_key.empty(); + } // Validates the current session bool validate_session(); - // Updates "now playing" status on Last.fm - bool update_now_playing(const TrackInfo& track); // Submits a track for scrobbling bool scrobble_track(const TrackInfo& track); // Sets API key and secret for authentication @@ -53,13 +51,16 @@ class LastfmApi // Sets session key for authenticated requests void set_session_key(const char* session_key); // Returns the current session key - std::string get_session_key() const { return m_session_key; } + std::string get_session_key() const { + std::lock_guard lock(m_session_mutex); + return m_session_key; + } + // Generates URL for user authentication + std::string get_auth_url() const; // Authenticates user with a token (asynchronous) void authenticate_async(const std::string& token, std::function callback); // Updates "now playing" status on Last.fm (asynchronous) void update_now_playing_async(const TrackInfo& track); - // Submits a track for scrobbling (asynchronous) - void scrobble_track_async(const TrackInfo& track); private: // API key for Last.fm @@ -68,6 +69,8 @@ class LastfmApi std::string m_api_secret; // Session key for authenticated requests std::string m_session_key; + // Mutex for thread-safe access to session key + mutable std::mutex m_session_mutex; // Executes an API request in a background thread void execute_async_request(const std::map& params, std::function callback); From e1dadba2c9e78afae468b7a8322d694b693031b3 Mon Sep 17 00:00:00 2001 From: avelytchko <919635+avelytchko@users.noreply.github.com> Date: Thu, 5 Mar 2026 20:04:49 +0100 Subject: [PATCH 2/2] Fix clang-format violations in inline methods --- COMMIT_MESSAGE.txt | 28 ++++++++++++++++++++++++ foobar2000/foo_mac_scrobble/lastfm_api.h | 10 +++++---- 2 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 COMMIT_MESSAGE.txt diff --git a/COMMIT_MESSAGE.txt b/COMMIT_MESSAGE.txt new file mode 100644 index 0000000..3e52ce9 --- /dev/null +++ b/COMMIT_MESSAGE.txt @@ -0,0 +1,28 @@ +refactor: fix thread safety and remove dead code + +Critical fixes: +- Add mutex protection for m_session_key to prevent race conditions +- Replace C-style cast with static_cast in write_callback() +- Fix clang-format violations in inline methods + +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 +- Fixed code formatting to pass clang-format checks + +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 diff --git a/foobar2000/foo_mac_scrobble/lastfm_api.h b/foobar2000/foo_mac_scrobble/lastfm_api.h index ee86507..319148d 100644 --- a/foobar2000/foo_mac_scrobble/lastfm_api.h +++ b/foobar2000/foo_mac_scrobble/lastfm_api.h @@ -38,9 +38,10 @@ class LastfmApi // Checks if the current session is authenticated bool is_authenticated() const; // Checks if a valid session exists - bool has_saved_session() const { + bool has_saved_session() const + { std::lock_guard lock(m_session_mutex); - return !m_session_key.empty(); + return !m_session_key.empty(); } // Validates the current session bool validate_session(); @@ -51,9 +52,10 @@ class LastfmApi // Sets session key for authenticated requests void set_session_key(const char* session_key); // Returns the current session key - std::string get_session_key() const { + std::string get_session_key() const + { std::lock_guard lock(m_session_mutex); - return m_session_key; + return m_session_key; } // Generates URL for user authentication std::string get_auth_url() const;