Skip to content

refactor: fix thread safety and remove dead code #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
avelytchko wants to merge 2 commits into
base: main
Choose a base branch
from
Open

refactor: fix thread safety and remove dead code #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4464ffb
refactor: fix thread safety and remove dead code
avelytchko Mar 5, 2026
e1dadba
Fix clang-format violations in inline methods
avelytchko Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions COMMIT_MESSAGE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
197 changes: 58 additions & 139 deletions foobar2000/foo_mac_scrobble/lastfm_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::mutex> lock(m_session_mutex);
if (!m_session_key.empty() && foo_lastfm::cfg_debug_enabled.get())
{
FB2K_console_formatter() << "Last.fm: Credentials changed - clearing session";
Expand All @@ -85,112 +86,36 @@ 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<std::string, std::string> 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<std::mutex> lock(m_session_mutex);
if (new_session_key != m_session_key)
{
int code = json_data["error"].get<int>();
std::string msg = json_data["message"].get<std::string>();
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<std::string, std::string> 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<std::mutex> lock(m_session_mutex);
return !m_session_key.empty();
}

bool LastfmApi::scrobble_track(const TrackInfo& track)
{
if (!is_authenticated())
return false;

std::map<std::string, std::string> 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<std::string, std::string> params;
{
std::lock_guard<std::mutex> 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())
Expand All @@ -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<std::mutex> lock(m_session_mutex);
if (m_session_key.empty())
return false;
}

std::map<std::string, std::string> params{
{"method", "auth.getSessionInfo"}, {"api_key", m_api_key}, {"sk", m_session_key}};
std::map<std::string, std::string> params;
{
std::lock_guard<std::mutex> 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;
Expand All @@ -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<std::mutex> 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<std::mutex> 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;
}

Expand Down Expand Up @@ -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<std::string*>(u)->append(static_cast<const char*>(c), t);
return t;
}

Expand Down Expand Up @@ -406,7 +345,10 @@ bool LastfmApi::send_api_request(const std::map<std::string, std::string>& param
{
if (method != "auth.getSession" && method != "auth.getToken")
{
m_session_key.clear();
{
std::lock_guard<std::mutex> lock(m_session_mutex);
m_session_key.clear();
}
foo_lastfm::cfg_session_key.set("");
if (foo_lastfm::cfg_debug_enabled.get())
{
Expand Down Expand Up @@ -478,6 +420,12 @@ bool LastfmApi::send_api_request(const std::map<std::string, std::string>& 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<std::string, std::string>& params,
std::function<void(bool success, const std::string& response)> callback)
{
Expand Down Expand Up @@ -532,11 +480,15 @@ void LastfmApi::authenticate_async(const std::string& token, std::function<void(
if (json_data.contains("session"))
{
auto session = json_data["session"];
m_session_key = session.value("key", "");
std::string session_key = session.value("key", "");
std::string username = session.value("name", "");
{
std::lock_guard<std::mutex> 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())
{
Expand Down Expand Up @@ -579,7 +531,10 @@ void LastfmApi::update_now_playing_async(const TrackInfo& track)
std::map<std::string, std::string> params;
params["method"] = "track.updateNowPlaying";
params["api_key"] = m_api_key;
params["sk"] = m_session_key;
{
std::lock_guard<std::mutex> lock(m_session_mutex);
params["sk"] = m_session_key;
}
params["artist"] = track.artist;
params["track"] = track.track;
params["album"] = track.album;
Expand All @@ -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<std::string, std::string> 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";
}
});
}
25 changes: 15 additions & 10 deletions foobar2000/foo_mac_scrobble/lastfm_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <ctime>
#include <curl/curl.h>
#include <map>
#include <mutex>
#include <string>
Comment on lines +13 to 14
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


class LastfmApi
Expand All @@ -34,32 +35,34 @@ 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<std::mutex> 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
void set_credentials(const char* api_key, const char* api_secret);
// 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<std::mutex> 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<void(bool success)> 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
Expand All @@ -68,6 +71,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<std::string, std::string>& params,
std::function<void(bool success, const std::string& response)> callback);
Expand Down