From f87c554423c86d28669b5757948662b75911cb4c Mon Sep 17 00:00:00 2001
From: Richard Maw <richard.maw@gmail.com>
Date: Sat, 17 May 2025 20:12:37 +0100
Subject: [PATCH] Make AssetLoader.requestAsset idempotent

This moves the check for whether there is already a request for the
asset inside the requestAsset method to remove a race condition
where AssetManager would synchronize checking whether there was a
request and then request it, which fails when multiple threads are
checking whether the asset is cached in parallel, all get the result
that there are no pending requests, and then submit multiple requests
when there should be only one.
---
 .../rptools/maptool/model/AssetLoader.java    | 21 ++++++++++++++++++-
 .../rptools/maptool/model/AssetManager.java   | 16 +-------------
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/src/main/java/net/rptools/maptool/model/AssetLoader.java b/src/main/java/net/rptools/maptool/model/AssetLoader.java
index d787c925db..ffec834c6c 100644
--- a/src/main/java/net/rptools/maptool/model/AssetLoader.java
+++ b/src/main/java/net/rptools/maptool/model/AssetLoader.java
@@ -86,6 +86,14 @@ public synchronized void removeAllRepositories() {
     repositoryStateMap.clear();
   }
 
+  /**
+   * Check whether there is currently a pending request for the given asset ID.
+   *
+   * <p>NOTE: This may be immediately incorrect by the time this function returns so decisions on
+   * whether to request an asset must not depend on the result.
+   *
+   * @return true if there is an incomplete request.
+   */
   public synchronized boolean isIdRequested(MD5Key id) {
     return requestedIdSet.contains(id);
   }
@@ -226,9 +234,20 @@ protected File getRepoIndexFile(String repository) {
     return new File(REPO_CACHE_DIR.getAbsolutePath() + "/" + new MD5Key(repository.getBytes()));
   }
 
-  public synchronized void requestAsset(MD5Key id) {
+  /**
+   * Submit a new request to retrieve the asset with the given ID if needed.
+   *
+   * @param id The ID of the asset to request.
+   * @return false if there was already a request, true if a new request was submitted.
+   */
+  public synchronized boolean requestAsset(MD5Key id) {
+    if (requestedIdSet.contains(id)) {
+      return false;
+    }
+
     retrievalThreadPool.submit(new ImageRetrievalRequest(id, createRequestQueue(id)));
     requestedIdSet.add(id);
+    return true;
   }
 
   public synchronized void completeRequest(MD5Key id) {
diff --git a/src/main/java/net/rptools/maptool/model/AssetManager.java b/src/main/java/net/rptools/maptool/model/AssetManager.java
index bf44066b95..53418cea69 100644
--- a/src/main/java/net/rptools/maptool/model/AssetManager.java
+++ b/src/main/java/net/rptools/maptool/model/AssetManager.java
@@ -142,18 +142,6 @@ public static void updateRepositoryList() {
     }
   }
 
-  /**
-   * Determine if the asset is currently being requested. While an asset is being loaded it will be
-   * marked as requested and this function will return true. Once the asset is done loading this
-   * function will return false and the asset will be available from the cache.
-   *
-   * @param key MD5Key of asset being requested
-   * @return True if asset is currently being requested, false otherwise
-   */
-  public static boolean isAssetRequested(MD5Key key) {
-    return assetLoader.isIdRequested(key);
-  }
-
   /**
    * Register a listener with the asset manager. The listener will be notified when the asset is
    * done loading.
@@ -302,9 +290,7 @@ public static void getAssetAsynchronously(
 
           // Let's get it from the server
           // As a last resort we request the asset from the server
-          if (!isAssetRequested(id)) {
-            requestAssetFromServer(id, listeners);
-          }
+          requestAssetFromServer(id, listeners);
         });
   }