Rewrote APState so connection happens on thread

The whole file is more thread-safe than before, with a few notable exceptions. This fixes a read-after-free issue where, when reconnecting after a disconnection, the client thread would attempt to lock a mutex owned and already destroyed by the main thread.

2 files changed, 418 insertions, 274 deletions

@@ -34,24 +34,31 @@ constexpr int AP_REVISION = 5;
 constexpr const char* CERT_STORE_PATH = "cacert.pem";
 constexpr int ITEM_HANDLING = 7;  // <- all
 
+constexpr int CONNECTION_TIMEOUT = 50000;  // 50 seconds
+constexpr int CONNECTION_BACKOFF_INTERVAL = 100;
+
 namespace {
 
 struct APState {
-  std::unique_ptr<APClient> apclient;
-
+  // Initialized on main thread
   bool initialized = false;
-
   TrackerFrame* tracker_frame = nullptr;
+  std::list<std::string> tracked_data_storage_keys;
 
-  bool client_active = false;
-  std::string status_message = "Not connected to Archipelago.";
+  // Client
   std::mutex client_mutex;
+  std::unique_ptr<APClient> apclient;
+
+  // Protected state
+  std::mutex state_mutex;
+
+  std::string status_message = "Not connected to Archipelago.";
 
   bool connected = false;
-  bool has_connection_result = false;
+  std::string connection_failure;
+  int remaining_loops = 0;
 
   std::string data_storage_prefix;
-  std::list<std::string> tracked_data_storage_keys;
   std::string victory_data_storage_key;
 
   std::string save_name;
@@ -81,21 +88,175 @@ struct APState {
   std::map<int, SunwarpMapping> sunwarp_mapping;
 
   void Connect(std::string server, std::string player, std::string password) {
+    Initialize();
+
+    {
+      std::lock_guard state_guard(state_mutex);
+      SetStatusMessage("Connecting to Archipelago server....");
+    }
+    TrackerLog(fmt::format("Connecting to Archipelago server ({})...", server));
+
+    // Creating and setting up the client has to all be done while holding the
+    // client mutex, so that the other thread doesn't try to poll before we add
+    // handlers, etc.
+    {
+      TrackerLog("Destroying old AP client...");
+
+      std::lock_guard client_guard(client_mutex);
+
+      if (apclient) {
+        DestroyClient();
+      }
+
+      std::string cert_store = "";
+      if (std::filesystem::exists(CERT_STORE_PATH)) {
+        cert_store = CERT_STORE_PATH;
+      }
+
+      apclient = std::make_unique<APClient>(ap_get_uuid(""), "Lingo", server,
+                                            cert_store);
+
+      {
+        std::lock_guard state_guard(state_mutex);
+
+        connected = false;
+        connection_failure.clear();
+        remaining_loops = CONNECTION_TIMEOUT / CONNECTION_BACKOFF_INTERVAL;
+
+        save_name.clear();
+        inventory.clear();
+        checked_locations.clear();
+        data_storage.clear();
+        player_pos = std::nullopt;
+        victory_data_storage_key.clear();
+        door_shuffle_mode = kNO_DOORS;
+        group_doors = false;
+        color_shuffle = false;
+        painting_shuffle = false;
+        painting_mapping.clear();
+        painting_codomain.clear();
+        mastery_requirement = 21;
+        level_2_requirement = 223;
+        location_checks = kNORMAL_LOCATIONS;
+        victory_condition = kTHE_END;
+        early_color_hallways = false;
+        pilgrimage_enabled = false;
+        pilgrimage_allows_roof_access = false;
+        pilgrimage_allows_paintings = false;
+        sunwarp_access = kSUNWARP_ACCESS_NORMAL;
+        sunwarp_shuffle = false;
+        sunwarp_mapping.clear();
+      }
+
+      apclient->set_room_info_handler(
+          [this, player, password]() { OnRoomInfo(player, password); });
+
+      apclient->set_location_checked_handler(
+          [this](const std::list<int64_t>& locations) {
+            OnLocationChecked(locations);
+          });
+
+      apclient->set_slot_disconnected_handler(
+          [this]() { OnSlotDisconnected(); });
+
+      apclient->set_socket_disconnected_handler(
+          [this]() { OnSocketDisconnected(); });
+
+      apclient->set_items_received_handler(
+          [this](const std::list<APClient::NetworkItem>& items) {
+            OnItemsReceived(items);
+          });
+
+      apclient->set_retrieved_handler(
+          [this](const std::map<std::string, nlohmann::json>& data) {
+            OnRetrieved(data);
+          });
+
+      apclient->set_set_reply_handler(
+          [this](const std::string& key, const nlohmann::json& value,
+                 const nlohmann::json&) { OnSetReply(key, value); });
+
+      apclient->set_slot_connected_handler(
+          [this, player, server](const nlohmann::json& slot_data) {
+            OnSlotConnected(player, server, slot_data);
+          });
+
+      apclient->set_slot_refused_handler(
+          [this](const std::list<std::string>& errors) {
+            OnSlotRefused(errors);
+          });
+    }
+  }
+
+  std::string GetStatusMessage() {
+    std::lock_guard state_guard(state_mutex);
+
+    return status_message;
+  }
+
+  bool HasCheckedGameLocation(int location_id) {
+    std::lock_guard state_guard(state_mutex);
+
+    return checked_locations.count(location_id);
+  }
+
+  bool HasCheckedHuntPanel(int location_id) {
+    std::lock_guard state_guard(state_mutex);
+
+    std::string key =
+        fmt::format("{}Hunt|{}", data_storage_prefix, location_id);
+    return data_storage.count(key) && std::any_cast<bool>(data_storage.at(key));
+  }
+
+  bool HasItem(int item_id, int quantity) {
+    return inventory.count(item_id) && inventory.at(item_id) >= quantity;
+  }
+
+  bool HasAchievement(const std::string& name) {
+    std::lock_guard state_guard(state_mutex);
+
+    std::string key =
+        fmt::format("{}Achievement|{}", data_storage_prefix, name);
+    return data_storage.count(key) && std::any_cast<bool>(data_storage.at(key));
+  }
+
+  const std::set<std::string>& GetCheckedPaintings() {
+    std::lock_guard state_guard(state_mutex);
+
+    std::string key = fmt::format("{}Paintings", data_storage_prefix);
+    if (!data_storage.count(key)) {
+      data_storage[key] = std::set<std::string>();
+    }
+
+    return std::any_cast<const std::set<std::string>&>(data_storage.at(key));
+  }
+
+  bool IsPaintingChecked(const std::string& painting_id) {
+    const auto& checked_paintings = GetCheckedPaintings();
+
+    std::lock_guard state_guard(state_mutex);
+
+    return checked_paintings.count(painting_id) ||
+           (painting_mapping.count(painting_id) &&
+            checked_paintings.count(painting_mapping.at(painting_id)));
+  }
+
+  std::string GetItemName(int id) { return apclient->get_item_name(id); }
+
+  bool HasReachedGoal() {
+    std::lock_guard state_guard(state_mutex);
+
+    return data_storage.count(victory_data_storage_key) &&
+           std::any_cast<int>(data_storage.at(victory_data_storage_key)) ==
+               30;  // CLIENT_GOAL
+  }
+
+ private:
+  void Initialize() {
     if (!initialized) {
       TrackerLog("Initializing APState...");
 
-      std::thread([this]() {
-        for (;;) {
-          {
-            std::lock_guard client_guard(client_mutex);
-            if (apclient) {
-              apclient->poll();
-            }
-          }
-
-          std::this_thread::sleep_for(std::chrono::milliseconds(100));
-        }
-      }).detach();
+      std::thread([this]() { Thread(); }).detach();
 
       for (int panel_id : GD_GetAchievementPanels()) {
         tracked_data_storage_keys.push_back(fmt::format(
@@ -114,130 +275,148 @@ struct APState {
 
       initialized = true;
     }
+  }
 
-    {
-      std::lock_guard client_guard(client_mutex);
-      SetStatusMessage("Connecting to Archipelago server....");
-    }
-    TrackerLog(fmt::format("Connecting to Archipelago server ({})...", server));
+  void Thread() {
+    std::string display_error;
 
-    {
-      TrackerLog("Destroying old AP client...");
+    for (;;) {
+      {
+        std::lock_guard client_guard(client_mutex);
+        if (apclient) {
+          apclient->poll();
 
-      std::lock_guard client_guard(client_mutex);
+          {
+            std::lock_guard state_guard(state_mutex);
 
-      if (apclient) {
-        DestroyClient();
+            if (!connected) {
+              if (!connection_failure.empty()) {
+                TrackerLog(connection_failure);
+
+                display_error = connection_failure;
+                connection_failure.clear();
+
+                DestroyClient();
+              } else {
+                remaining_loops--;
+
+                if (remaining_loops <= 0) {
+                  DestroyClient();
+
+                  SetStatusMessage("Disconnected from Archipelago.");
+                  TrackerLog("Timeout while connecting to Archipelago server.");
+
+                  display_error =
+                      "Timeout while connecting to Archipelago server.";
+                }
+              }
+            }
+          }
+        }
       }
 
-      std::string cert_store = "";
-      if (std::filesystem::exists(CERT_STORE_PATH)) {
-        cert_store = CERT_STORE_PATH;
+      if (!display_error.empty()) {
+        wxMessageBox(display_error, "Connection failed", wxOK | wxICON_ERROR);
+        display_error.clear();
       }
 
-      apclient = std::make_unique<APClient>(ap_get_uuid(""), "Lingo", server,
-                                            cert_store);
+      std::this_thread::sleep_for(std::chrono::milliseconds(100));
     }
+  }
+
+  void OnRoomInfo(std::string player, std::string password) {
+    {
+      std::lock_guard state_guard(state_mutex);
 
-    save_name.clear();
-    inventory.clear();
-    checked_locations.clear();
-    data_storage.clear();
-    player_pos = std::nullopt;
-    victory_data_storage_key.clear();
-    door_shuffle_mode = kNO_DOORS;
-    group_doors = false;
-    color_shuffle = false;
-    painting_shuffle = false;
-    painting_mapping.clear();
-    painting_codomain.clear();
-    mastery_requirement = 21;
-    level_2_requirement = 223;
-    location_checks = kNORMAL_LOCATIONS;
-    victory_condition = kTHE_END;
-    early_color_hallways = false;
-    pilgrimage_enabled = false;
-    pilgrimage_allows_roof_access = false;
-    pilgrimage_allows_paintings = false;
-    sunwarp_access = kSUNWARP_ACCESS_NORMAL;
-    sunwarp_shuffle = false;
-    sunwarp_mapping.clear();
-
-    std::mutex connection_mutex;
-    connected = false;
-    has_connection_result = false;
-
-    apclient->set_room_info_handler([this, player, password]() {
       inventory.clear();
 
-      TrackerLog(fmt::format(
-          "Connected to Archipelago server. Authenticating as {} {}", player,
-          (password.empty() ? "without password"
-                            : "with password " + password)));
       SetStatusMessage("Connected to Archipelago server. Authenticating...");
+    }
 
-      apclient->ConnectSlot(player, password, ITEM_HANDLING, {"Tracker"},
-                            {AP_MAJOR, AP_MINOR, AP_REVISION});
-    });
+    TrackerLog(fmt::format(
+        "Connected to Archipelago server. Authenticating as {} {}", player,
+        (password.empty() ? "without password" : "with password " + password)));
 
-    apclient->set_location_checked_handler(
-        [this](const std::list<int64_t>& locations) {
-          for (const int64_t location_id : locations) {
-            checked_locations.insert(location_id);
-            TrackerLog(fmt::format("Location: {}", location_id));
-          }
+    apclient->ConnectSlot(player, password, ITEM_HANDLING, {"Tracker"},
+                          {AP_MAJOR, AP_MINOR, AP_REVISION});
+  }
 
-          RefreshTracker(false);
-        });
+  void OnLocationChecked(const std::list<int64_t>& locations) {
+    {
+      std::lock_guard state_guard(state_mutex);
 
-    apclient->set_slot_disconnected_handler([this]() {
-      SetStatusMessage(
-          "Disconnected from Archipelago. Attempting to reconnect...");
-      TrackerLog(
-          "Slot disconnected from Archipelago. Attempting to reconnect...");
-    });
+      for (const int64_t location_id : locations) {
+        checked_locations.insert(location_id);
+        TrackerLog(fmt::format("Location: {}", location_id));
+      }
+    }
 
-    apclient->set_socket_disconnected_handler([this]() {
-      SetStatusMessage(
-          "Disconnected from Archipelago. Attempting to reconnect...");
-      TrackerLog(
-          "Socket disconnected from Archipelago. Attempting to reconnect...");
-    });
-
-    apclient->set_items_received_handler(
-        [this](const std::list<APClient::NetworkItem>& items) {
-          for (const APClient::NetworkItem& item : items) {
-            inventory[item.item]++;
-            TrackerLog(fmt::format("Item: {}", item.item));
-          }
+    RefreshTracker(false);
+  }
 
-          RefreshTracker(false);
-        });
+  void OnSlotDisconnected() {
+    std::lock_guard state_guard(state_mutex);
 
-    apclient->set_retrieved_handler(
-        [this](const std::map<std::string, nlohmann::json>& data) {
-          for (const auto& [key, value] : data) {
-            HandleDataStorage(key, value);
-          }
+    SetStatusMessage(
+        "Disconnected from Archipelago. Attempting to reconnect...");
+    TrackerLog(
+        "Slot disconnected from Archipelago. Attempting to reconnect...");
+  }
 
-          RefreshTracker(false);
-        });
+  void OnSocketDisconnected() {
+    std::lock_guard state_guard(state_mutex);
 
-    apclient->set_set_reply_handler([this](const std::string& key,
-                                           const nlohmann::json& value,
-                                           const nlohmann::json&) {
+    SetStatusMessage(
+        "Disconnected from Archipelago. Attempting to reconnect...");
+    TrackerLog(
+        "Socket disconnected from Archipelago. Attempting to reconnect...");
+  }
+
+  void OnItemsReceived(const std::list<APClient::NetworkItem>& items) {
+    {
+      std::lock_guard state_guard(state_mutex);
+
+      for (const APClient::NetworkItem& item : items) {
+        inventory[item.item]++;
+        TrackerLog(fmt::format("Item: {}", item.item));
+      }
+    }
+
+    RefreshTracker(false);
+  }
+
+  void OnRetrieved(const std::map<std::string, nlohmann::json>& data) {
+    {
+      std::lock_guard state_guard(state_mutex);
+
+      for (const auto& [key, value] : data) {
+        HandleDataStorage(key, value);
+      }
+    }
+
+    RefreshTracker(false);
+  }
+
+  void OnSetReply(const std::string& key, const nlohmann::json& value) {
+    {
+      std::lock_guard state_guard(state_mutex);
       HandleDataStorage(key, value);
-      RefreshTracker(false);
-    });
+    }
+
+    RefreshTracker(false);
+  }
+
+  void OnSlotConnected(std::string player, std::string server,
+                       const nlohmann::json& slot_data) {
+    IPC_SetTrackerSlot(server, player);
+
+    TrackerLog("Connected to Archipelago!");
+
+    {
+      std::lock_guard state_guard(state_mutex);
 
-    apclient->set_slot_connected_handler([this, player, server,
-                                          &connection_mutex](
-                                             const nlohmann::json& slot_data) {
       SetStatusMessage(
           fmt::format("Connected to Archipelago! ({}@{}).", player, server));
-      TrackerLog("Connected to Archipelago!");
-
-      IPC_SetTrackerSlot(server, player);
 
       save_name = fmt::format("zzAP_{}_{}.save", apclient->get_seed(),
                               apclient->get_player_number());
@@ -248,8 +427,8 @@ struct APState {
         group_doors = slot_data.contains("group_doors") &&
                       slot_data["group_doors"].get<int>() == 1;
       } else {
-        // If group_doors doesn't exist yet, that means kPANELS_MODE is actually
-        // kSIMPLE_DOORS.
+        // If group_doors doesn't exist yet, that means kPANELS_MODE is
+        // actually kSIMPLE_DOORS.
         if (door_shuffle_mode == kPANELS_MODE) {
           door_shuffle_mode = kDOORS_MODE;
           group_doors = true;
@@ -314,112 +493,53 @@ struct APState {
       apclient->Get(corrected_keys);
       apclient->SetNotify(corrected_keys);
 
-      ResetReachabilityRequirements();
-      RefreshTracker(true);
-
-      {
-        std::lock_guard connection_lock(connection_mutex);
-        if (!has_connection_result) {
-          connected = true;
-          has_connection_result = true;
-        }
-      }
-    });
-
-    apclient->set_slot_refused_handler(
-        [this, &connection_mutex](const std::list<std::string>& errors) {
-          {
-            std::lock_guard connection_lock(connection_mutex);
-            connected = false;
-            has_connection_result = true;
-          }
-
-          SetStatusMessage("Disconnected from Archipelago.");
-
-          std::vector<std::string> error_messages;
-          error_messages.push_back("Could not connect to Archipelago.");
-
-          for (const std::string& error : errors) {
-            if (error == "InvalidSlot") {
-              error_messages.push_back("Invalid player name.");
-            } else if (error == "InvalidGame") {
-              error_messages.push_back(
-                  "The specified player is not playing Lingo.");
-            } else if (error == "IncompatibleVersion") {
-              error_messages.push_back(
-                  "The Archipelago server is not the correct version for this "
-                  "client.");
-            } else if (error == "InvalidPassword") {
-              error_messages.push_back("Incorrect password.");
-            } else if (error == "InvalidItemsHandling") {
-              error_messages.push_back(
-                  "Invalid item handling flag. This is a bug with the tracker. "
-                  "Please report it to the lingo-ap-tracker GitHub.");
-            } else {
-              error_messages.push_back("Unknown error.");
-            }
-          }
-
-          std::string full_message = hatkirby::implode(error_messages, " ");
-          TrackerLog(full_message);
-
-          wxMessageBox(full_message, "Connection failed", wxOK | wxICON_ERROR);
-        });
-
-    client_active = true;
-
-    int timeout = 5000;  // 5 seconds
-    int interval = 100;
-    int remaining_loops = timeout / interval;
-    while (true) {
-      {
-        std::lock_guard connection_lock(connection_mutex);
-        if (has_connection_result) {
-          break;
-        }
-      }
-
-      if (interval == 0) {
-        DestroyClient();
+      connected = true;
+    }
 
-        {
-          std::lock_guard client_guard(client_mutex);
-          SetStatusMessage("Disconnected from Archipelago.");
-        }
-        TrackerLog("Timeout while connecting to Archipelago server.");
-        wxMessageBox("Timeout while connecting to Archipelago server.",
-                     "Connection failed", wxOK | wxICON_ERROR);
-
-        {
-          std::lock_guard connection_lock(connection_mutex);
-          connected = false;
-          has_connection_result = true;
-        }
+    ResetReachabilityRequirements();
+    RefreshTracker(true);
+  }
 
-        break;
+  void OnSlotRefused(const std::list<std::string>& errors) {
+    std::vector<std::string> error_messages;
+    error_messages.push_back("Could not connect to Archipelago.");
+
+    for (const std::string& error : errors) {
+      if (error == "InvalidSlot") {
+        error_messages.push_back("Invalid player name.");
+      } else if (error == "InvalidGame") {
+        error_messages.push_back("The specified player is not playing Lingo.");
+      } else if (error == "IncompatibleVersion") {
+        error_messages.push_back(
+            "The Archipelago server is not the correct version for this "
+            "client.");
+      } else if (error == "InvalidPassword") {
+        error_messages.push_back("Incorrect password.");
+      } else if (error == "InvalidItemsHandling") {
+        error_messages.push_back(
+            "Invalid item handling flag. This is a bug with the tracker. "
+            "Please report it to the lingo-ap-tracker GitHub.");
+      } else {
+        error_messages.push_back("Unknown error.");
       }
-
-      std::this_thread::sleep_for(std::chrono::milliseconds(100));
-
-      interval--;
     }
 
-    if (connected) {
-      client_active = false;
-    }
-  }
+    {
+      std::lock_guard state_guard(state_mutex);
+      connection_failure = hatkirby::implode(error_messages, " ");
 
-  std::string GetStatusMessage() {
-    std::lock_guard client_guard(client_mutex);
-    return status_message;
+      SetStatusMessage("Disconnected from Archipelago.");
+    }
   }
 
+  // Assumes state mutex is locked.
   void SetStatusMessage(std::string msg) {
     status_message = std::move(msg);
 
     tracker_frame->UpdateStatusMessage();
   }
 
+  // Assumes state mutex is locked.
   void HandleDataStorage(const std::string& key, const nlohmann::json& value) {
     if (value.is_boolean()) {
       data_storage[key] = value.get<bool>();
@@ -461,46 +581,18 @@ struct APState {
     }
   }
 
-  bool HasCheckedGameLocation(int location_id) {
-    return checked_locations.count(location_id);
-  }
-
-  bool HasCheckedHuntPanel(int location_id) {
-    std::string key =
-        fmt::format("{}Hunt|{}", data_storage_prefix, location_id);
-    return data_storage.count(key) && std::any_cast<bool>(data_storage.at(key));
-  }
-
-  bool HasItem(int item_id, int quantity) {
-    return inventory.count(item_id) && inventory.at(item_id) >= quantity;
-  }
+  // State mutex should NOT be locked.
+  void RefreshTracker(bool reset) {
+    TrackerLog("Refreshing display...");
 
-  bool HasAchievement(const std::string& name) {
-    std::string key =
-        fmt::format("{}Achievement|{}", data_storage_prefix, name);
-    return data_storage.count(key) && std::any_cast<bool>(data_storage.at(key));
-  }
+    std::string prev_msg;
+    {
+      std::lock_guard state_guard(state_mutex);
 
-  const std::set<std::string>& GetCheckedPaintings() {
-    std::string key = fmt::format("{}Paintings", data_storage_prefix);
-    if (!data_storage.count(key)) {
-      data_storage[key] = std::set<std::string>();
+      prev_msg = status_message;
+      SetStatusMessage(fmt::format("{} Recalculating...", status_message));
     }
 
-    return std::any_cast<const std::set<std::string>&>(data_storage.at(key));
-  }
-
-  bool IsPaintingChecked(const std::string& painting_id) {
-    const auto& checked_paintings = GetCheckedPaintings();
-
-    return checked_paintings.count(painting_id) ||
-           (painting_mapping.count(painting_id) &&
-            checked_paintings.count(painting_mapping.at(painting_id)));
-  }
-
-  void RefreshTracker(bool reset) {
-    TrackerLog("Refreshing display...");
-
     RecalculateReachability();
 
     if (reset) {
@@ -508,27 +600,15 @@ struct APState {
     } else {
       tracker_frame->UpdateIndicators();
     }
-  }
-
-  int64_t GetItemId(const std::string& item_name) {
-    int64_t ap_id = apclient->get_item_id(item_name);
-    if (ap_id == APClient::INVALID_NAME_ID) {
-      TrackerLog(fmt::format("Could not find AP item ID for {}", item_name));
-    }
 
-    return ap_id;
-  }
-
-  std::string GetItemName(int id) { return apclient->get_item_name(id); }
+    {
+      std::lock_guard state_guard(state_mutex);
 
-  bool HasReachedGoal() {
-    return data_storage.count(victory_data_storage_key) &&
-           std::any_cast<int>(data_storage.at(victory_data_storage_key)) ==
-               30;  // CLIENT_GOAL
+      SetStatusMessage(prev_msg);
+    }
   }
 
   void DestroyClient() {
-    client_active = false;
     apclient->reset();
     apclient.reset();
   }
@@ -549,7 +629,11 @@ void AP_Connect(std::string server, std::string player, std::string password) {
 
 std::string AP_GetStatusMessage() { return GetState().GetStatusMessage(); }
 
-std::string AP_GetSaveName() { return GetState().save_name; }
+std::string AP_GetSaveName() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
+  return GetState().save_name;
+}
 
 bool AP_HasCheckedGameLocation(int location_id) {
   return GetState().HasCheckedGameLocation(location_id);
@@ -567,19 +651,37 @@ std::string AP_GetItemName(int item_id) {
   return GetState().GetItemName(item_id);
 }
 
-DoorShuffleMode AP_GetDoorShuffleMode() { return GetState().door_shuffle_mode; }
+DoorShuffleMode AP_GetDoorShuffleMode() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
+  return GetState().door_shuffle_mode;
+}
 
-bool AP_AreDoorsGrouped() { return GetState().group_doors; }
+bool AP_AreDoorsGrouped() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
+  return GetState().group_doors;
+}
 
-bool AP_IsColorShuffle() { return GetState().color_shuffle; }
+bool AP_IsColorShuffle() {
+  std::lock_guard state_guard(GetState().state_mutex);
 
-bool AP_IsPaintingShuffle() { return GetState().painting_shuffle; }
+  return GetState().color_shuffle;
+}
+
+bool AP_IsPaintingShuffle() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
+  return GetState().painting_shuffle;
+}
 
 const std::map<std::string, std::string>& AP_GetPaintingMapping() {
   return GetState().painting_mapping;
 }
 
 bool AP_IsPaintingMappedTo(const std::string& painting_id) {
+  std::lock_guard state_guard(GetState().state_mutex);
+
   return GetState().painting_codomain.count(painting_id);
 }
 
@@ -591,11 +693,21 @@ bool AP_IsPaintingChecked(const std::string& painting_id) {
   return GetState().IsPaintingChecked(painting_id);
 }
 
-int AP_GetMasteryRequirement() { return GetState().mastery_requirement; }
+int AP_GetMasteryRequirement() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
+  return GetState().mastery_requirement;
+}
 
-int AP_GetLevel2Requirement() { return GetState().level_2_requirement; }
+int AP_GetLevel2Requirement() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
+  return GetState().level_2_requirement;
+}
 
 bool AP_IsLocationVisible(int classification) {
+  std::lock_guard state_guard(GetState().state_mutex);
+
   int world_state = 0;
 
   switch (GetState().location_checks) {
@@ -621,6 +733,8 @@ bool AP_IsLocationVisible(int classification) {
 }
 
 VictoryCondition AP_GetVictoryCondition() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
   return GetState().victory_condition;
 }
 
@@ -628,21 +742,41 @@ bool AP_HasAchievement(const std::string& achievement_name) {
   return GetState().HasAchievement(achievement_name);
 }
 
-bool AP_HasEarlyColorHallways() { return GetState().early_color_hallways; }
+bool AP_HasEarlyColorHallways() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
+  return GetState().early_color_hallways;
+}
+
+bool AP_IsPilgrimageEnabled() {
+  std::lock_guard state_guard(GetState().state_mutex);
 
-bool AP_IsPilgrimageEnabled() { return GetState().pilgrimage_enabled; }
+  return GetState().pilgrimage_enabled;
+}
 
 bool AP_DoesPilgrimageAllowRoofAccess() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
   return GetState().pilgrimage_allows_roof_access;
 }
 
 bool AP_DoesPilgrimageAllowPaintings() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
   return GetState().pilgrimage_allows_paintings;
 }
 
-SunwarpAccess AP_GetSunwarpAccess() { return GetState().sunwarp_access; }
+SunwarpAccess AP_GetSunwarpAccess() {
+  std::lock_guard state_guard(GetState().state_mutex);
 
-bool AP_IsSunwarpShuffle() { return GetState().sunwarp_shuffle; }
+  return GetState().sunwarp_access;
+}
+
+bool AP_IsSunwarpShuffle() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
+  return GetState().sunwarp_shuffle;
+}
 
 const std::map<int, SunwarpMapping>& AP_GetSunwarpMapping() {
   return GetState().sunwarp_mapping;
@@ -651,5 +785,7 @@ const std::map<int, SunwarpMapping>& AP_GetSunwarpMapping() {
 bool AP_HasReachedGoal() { return GetState().HasReachedGoal(); }
 
 std::optional<std::tuple<int, int>> AP_GetPlayerPosition() {
+  std::lock_guard state_guard(GetState().state_mutex);
+
   return GetState().player_pos;
 }
@@ -51,8 +51,13 @@ bool AP_HasCheckedGameLocation(int location_id);
 
 bool AP_HasCheckedHuntPanel(int location_id);
 
+// This doesn't lock the state mutex, for speed, so it must ONLY be called from
+// RecalculateReachability, which is only called from the APState thread anyway.
 bool AP_HasItem(int item_id, int quantity = 1);
 
+// This doesn't lock the client mutex because it is ONLY to be called from
+// RecalculateReachability, which is only called from within a client callback
+// anyway.
 std::string AP_GetItemName(int item_id);
 
 DoorShuffleMode AP_GetDoorShuffleMode();
@@ -63,10 +68,12 @@ bool AP_IsColorShuffle();
 
 bool AP_IsPaintingShuffle();
 
+// WARNING: Not thread-safe!
 const std::map<std::string, std::string>& AP_GetPaintingMapping();
 
 bool AP_IsPaintingMappedTo(const std::string& painting_id);
 
+// WARNING: Not thread-safe!
 const std::set<std::string>& AP_GetCheckedPaintings();
 
 bool AP_IsPaintingChecked(const std::string& painting_id);
@@ -93,6 +100,7 @@ SunwarpAccess AP_GetSunwarpAccess();
 
 bool AP_IsSunwarpShuffle();
 
+// WARNING: Not thread-safe!
 const std::map<int, SunwarpMapping>& AP_GetSunwarpMapping();
 
 bool AP_HasReachedGoal();