Code cleanup, less futures, more exceptions when needed

api/src/main/java/org/geysermc/geyser/api/pack/PackCodec.java

@@ -68,7 +68,7 @@ public abstract class PackCodec {
      * @return the new resource pack
      */
     @NonNull
-    protected abstract ResourcePack create();
+    public abstract ResourcePack create();
 
     /**
      * Creates a new pack provider from the given path.

api/src/main/java/org/geysermc/geyser/api/pack/UrlPackCodec.java

@@ -36,8 +36,6 @@ import org.checkerframework.checker.nullness.qual.NonNull;
  *     <li>be a direct download link to a .zip or .mcpack resource pack</li>
  *     <li>use the application type `application/zip` and set a correct content length</li>
  * </ul>
- *
- * Additionally, the resource pack must be zipped in a folder enclosing the resource pack, instead of the resource pack being at the root of the zip.
  */
 public abstract class UrlPackCodec extends PackCodec {
 

core/src/main/java/org/geysermc/geyser/GeyserImpl.java

@@ -650,8 +650,6 @@ public class GeyserImpl implements GeyserApi {
             this.erosionUnixListener.close();
         }
 
-        // todo check
-        //Registries.RESOURCE_PACKS.get().clear();
         ResourcePackLoader.clear();
 
         this.setEnabled(false);

core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java

@@ -319,7 +319,7 @@ public class UpstreamPacketHandler extends LoggingPacketHandler {
 
         // If a remote pack ends up here, that usually implies that a platform was not able to download the pack
         if (codec instanceof UrlPackCodec urlPackCodec) {
-            ResourcePackLoader.checkPack(urlPackCodec);
+            ResourcePackLoader.testUrlPack(urlPackCodec);
         }
 
         data.setChunkIndex(packet.getChunkIndex());

core/src/main/java/org/geysermc/geyser/pack/url/GeyserUrlPackCodec.java

@@ -28,56 +28,46 @@ package org.geysermc.geyser.pack.url;
 import lombok.Getter;
 import org.checkerframework.checker.nullness.qual.NonNull;
 import org.checkerframework.checker.nullness.qual.Nullable;
-import org.geysermc.geyser.GeyserImpl;
+import org.geysermc.geyser.api.pack.PathPackCodec;
 import org.geysermc.geyser.api.pack.ResourcePack;
 import org.geysermc.geyser.api.pack.UrlPackCodec;
-import org.geysermc.geyser.pack.path.GeyserPathPackCodec;
 import org.geysermc.geyser.registry.loader.ResourcePackLoader;
 
 import java.io.IOException;
 import java.nio.channels.SeekableByteChannel;
-import java.nio.file.Path;
+import java.util.Objects;
 
 public class GeyserUrlPackCodec extends UrlPackCodec {
-    private final String url;
+    private final @NonNull String url;
-    private final String contentKey;
+    private final @Nullable String contentKey;
     @Getter
-    private GeyserPathPackCodec fallback;
+    private PathPackCodec fallback;
 
     public GeyserUrlPackCodec(String url) throws IllegalArgumentException {
-        this(url, "");
+        this(url, null);
     }
 
     public GeyserUrlPackCodec(@NonNull String url, @Nullable String contentKey) throws IllegalArgumentException {
-        //noinspection ConstantValue - need to enforce
+        Objects.requireNonNull(url, "url cannot be null");
-        if (url == null) {
-            throw new IllegalArgumentException("Url cannot be nulL!");
-        }
         this.url = url;
-        this.contentKey = contentKey == null ? "" : contentKey;
+        this.contentKey = contentKey;
     }
 
     @Override
     public byte @NonNull [] sha256() {
-        if (this.fallback == null) {
+        Objects.requireNonNull(fallback, "must call #create() before attempting to get the sha256!");
-            throw new IllegalStateException("Fallback pack not initialized! Needs to be created first.");
-        }
         return fallback.sha256();
     }
 
     @Override
     public long size() {
-        if (this.fallback == null) {
+        Objects.requireNonNull(fallback, "must call #create() before attempting to get the size!");
-            throw new IllegalStateException("Fallback pack not initialized! Needs to be created first.");
-        }
         return fallback.size();
     }
 
     @Override
     public @NonNull SeekableByteChannel serialize(@NonNull ResourcePack resourcePack) throws IOException {
-        if (this.fallback == null) {
+        Objects.requireNonNull(fallback, "must call #create() before attempting to serialize!!");
-            throw new IllegalStateException("Fallback pack not initialized! Needs to be created first.");
-        }
         return fallback.serialize(resourcePack);
     }
 
@@ -86,17 +76,15 @@ public class GeyserUrlPackCodec extends UrlPackCodec {
     public ResourcePack create() {
         if (this.fallback == null) {
             try {
-                final Path downloadedPack = ResourcePackLoader.downloadPack(url, false).whenComplete((pack, throwable) -> {
+                ResourcePackLoader.downloadPack(url, false).whenComplete((pack, throwable) -> {
                     if (throwable != null) {
-                        GeyserImpl.getInstance().getLogger().error("Failed to download pack from " + url + " due to " + throwable.getMessage());
+                        throw new IllegalArgumentException(throwable);
-                        if (GeyserImpl.getInstance().getConfig().isDebugMode()) {
+                    } else if (pack != null) {
-                            GeyserImpl.getInstance().getLogger().error("full error: " + throwable);
+                        this.fallback = pack;
                     }
-                    }
+                });
-                }).join();
-                this.fallback = new GeyserPathPackCodec(downloadedPack);
             } catch (Exception e) {
-                throw new IllegalArgumentException("Failed to download pack from " + url, e);
+                throw new IllegalArgumentException("Failed to download pack from the url %s (reason: %s)!".formatted(url, e.getMessage()));
             }
         }
         return ResourcePackLoader.readPack(this);
@@ -109,6 +97,6 @@ public class GeyserUrlPackCodec extends UrlPackCodec {
 
     @Override
     public @NonNull String contentKey() {
-        return this.contentKey;
+        return this.contentKey != null ? contentKey : "";
     }
 }

core/src/main/java/org/geysermc/geyser/registry/loader/ResourcePackLoader.java

@@ -25,10 +25,13 @@
 
 package org.geysermc.geyser.registry.loader;
 
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
 import it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.geysermc.geyser.GeyserImpl;
 import org.geysermc.geyser.api.event.lifecycle.GeyserLoadResourcePacksEvent;
+import org.geysermc.geyser.api.pack.PathPackCodec;
 import org.geysermc.geyser.api.pack.ResourcePack;
 import org.geysermc.geyser.api.pack.ResourcePackManifest;
 import org.geysermc.geyser.api.pack.UrlPackCodec;
@@ -49,12 +52,9 @@ import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.PathMatcher;
-import java.util.ArrayList;
+import java.util.*;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -70,7 +70,9 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
      * Used to keep track of remote resource packs that the client rejected.
      * If a client rejects such a pack, it falls back to the old method, and Geyser serves a cached variant.
      */
-    private static final Set<UrlPackCodec> brokenPacks = new HashSet<>();
+    private static final Cache<String, UrlPackCodec> CACHED_FAILED_PACKS = CacheBuilder.newBuilder()
+            .expireAfterWrite(1, TimeUnit.HOURS)
+            .build();
 
     static final PathMatcher PACK_MATCHER = FileSystems.getDefault().getPathMatcher("glob:**.{zip,mcpack}");
 
@@ -169,9 +171,6 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
      */
     public static GeyserResourcePack readPack(GeyserUrlPackCodec codec) throws IllegalArgumentException {
         Path path = codec.getFallback().path();
-        if (!PACK_MATCHER.matches(path)) {
-            throw new IllegalArgumentException("The url " + codec.url() + " did not provide a valid resource pack! Please check the url and try again.");
-        }
 
         ResourcePackManifest manifest = readManifest(path, codec.url());
         String contentKey = codec.contentKey();
@@ -216,9 +215,9 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
     }
 
     private Map<String, ResourcePack> loadRemotePacks() {
+        // Unable to make this a static variable, as the test would fail
         final Path cachedCdnPacksDirectory = GeyserImpl.getInstance().getBootstrap().getConfigFolder().resolve("cache").resolve("remote_packs");
 
-        // Download CDN packs to get the pack uuid's
         if (!Files.exists(cachedCdnPacksDirectory)) {
             try {
                 Files.createDirectories(cachedCdnPacksDirectory);
@@ -246,57 +245,65 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
         return packMap;
     }
 
-    public static void checkPack(UrlPackCodec codec) {
+    /**
-        if (!brokenPacks.contains(codec)) {
+     * Used when a Bedrock client requests a Bedrock resource pack from the server when it should be downloading it
-            brokenPacks.add(codec);
+     * from a remote provider. Since this would be called each time a Bedrock client requests a piece of the Bedrock pack,
-            GeyserImpl.getInstance().getLogger().warning("Received a request for a remote pack that the client should have already downloaded! " +
+     * this uses a cache to ensure we aren't re-checking a dozen times.
-                    "Is the pack at the URL " + codec.url() + " still available?");
+     *
+     * @param codec the codec of the resource pack that wasn't successfully downloaded by a Bedrock client.
+     */
+    public static void testUrlPack(UrlPackCodec codec) {
+        if (CACHED_FAILED_PACKS.getIfPresent(codec.url()) == null) {
+            CACHED_FAILED_PACKS.put(codec.url(), codec);
+            GeyserImpl.getInstance().getLogger().warning("A client was not able to download the resource pack at %s. Is it still available? Running check now:");
             downloadPack(codec.url(), true);
         }
     }
 
-    public static CompletableFuture<@Nullable Path> downloadPack(String url, boolean checking) throws IllegalArgumentException {
+    public static CompletableFuture<@Nullable PathPackCodec> downloadPack(String url, boolean testing) throws IllegalArgumentException {
-        return WebUtils.checkUrlAndDownloadRemotePack(url, checking).whenCompleteAsync((cachedPath, throwable) -> {
+        return CompletableFuture.supplyAsync(() -> {
-            if (cachedPath == null) {
+            Path path = WebUtils.checkUrlAndDownloadRemotePack(url, testing);
-                // already warned about in WebUtils
-                return;
-            }
 
-            if (throwable != null) {
+            // Already warned about these above
-                GeyserImpl.getInstance().getLogger().error("Failed to download resource pack! ", throwable);
+            if (path == null) {
-                return;
+                return null;
             }
 
             // Check if the pack is a .zip or .mcpack file
-            if (!PACK_MATCHER.matches(cachedPath)) {
+            if (!PACK_MATCHER.matches(path)) {
                 throw new IllegalArgumentException("Invalid pack format! Not a .zip or .mcpack file.");
             }
 
-            if (checking) {
             try {
-                    Files.delete(cachedPath);
+                try (ZipFile zip = new ZipFile(path.toFile())) {
-                } catch (IOException e) {
-                    throw new IllegalArgumentException("Could not delete debug pack! " + e.getMessage(), e);
-                }
-            }
-
-            try {
-                try (ZipFile zip = new ZipFile(cachedPath.toFile())) {
                     if (zip.stream().noneMatch(x -> x.getName().contains("manifest.json"))) {
-                        throw new IllegalArgumentException(url + " does not contain a manifest file.");
+                        throw new IllegalArgumentException("The pack at the url " + url + " does not contain a manifest file!");
                     }
 
-//                    // Check if a "manifest.json" or "pack_manifest.json" file is located directly in the zip... does not work otherwise.
+                    // Check if a "manifest.json" or "pack_manifest.json" file is located directly in the zip... does not work otherwise.
-//                    // (something like MyZip.zip/manifest.json) will not, but will if it's a subfolder (MyPack.zip/MyPack/manifest.json)
+                    // (something like MyZip.zip/manifest.json) will not, but will if it's a subfolder (MyPack.zip/MyPack/manifest.json)
-//                    if (zip.getEntry("manifest.json") != null || zip.getEntry("pack_manifest.json") != null) {
+                    if (zip.getEntry("manifest.json") != null || zip.getEntry("pack_manifest.json") != null) {
-//                        GeyserImpl.getInstance().getLogger().debug("The remote resource pack from " + url + " contains a manifest.json file at the root of the zip file. " +
+                        if (testing) {
-//                                "This is not supported for remote packs, and will cause Bedrock clients to fall back to request the pack from the server. " +
+                            GeyserImpl.getInstance().getLogger().info("The remote resource pack from " + url + " contains a manifest.json file at the root of the zip file. " +
-//                                "Please put the pack file in a subfolder, and provide that zip in the URL.");
+                                    "This may not work for remote packs, and could cause Bedrock clients to fall back to request the pack from the server. " +
-//                    }
+                                    "Please put the pack file in a subfolder, and provide that zip in the URL.");
+                        }
+                    }
                 }
             } catch (IOException e) {
                 throw new IllegalArgumentException(GeyserLocale.getLocaleStringLog("geyser.resource_pack.broken", url), e);
             }
+
+            if (testing) {
+                try {
+                    Files.delete(path);
+                    return null;
+                } catch (IOException e) {
+                    throw new IllegalStateException("Could not delete debug pack! " + e.getMessage(), e);
+                }
+            }
+
+            return new GeyserPathPackCodec(path);
         });
     }
 
@@ -305,7 +312,7 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
 
         // Now: let's clean up broken remote packs, so we don't cache them
         Path location = GeyserImpl.getInstance().getBootstrap().getConfigFolder().resolve("cache").resolve("remote_packs");
-        brokenPacks.forEach(codec -> {
+        for (UrlPackCodec codec : CACHED_FAILED_PACKS.asMap().values()) {
             int hash = codec.url().hashCode();
             Path packLocation = location.resolve(hash + ".zip");
             Path packMetadata = packLocation.resolveSibling(hash + ".metadata");
@@ -320,6 +327,6 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
             } catch (IOException e) {
                 GeyserImpl.getInstance().getLogger().error("Could not delete broken cached resource packs! " + e);
             }
-        });
+        }
     }
 }

core/src/main/java/org/geysermc/geyser/util/WebUtils.java

@@ -33,11 +33,7 @@ import org.geysermc.geyser.GeyserLogger;
 import javax.naming.directory.Attribute;
 import javax.naming.directory.InitialDirContext;
 import java.io.*;
-import java.net.HttpURLConnection;
+import java.net.*;
-import java.net.MalformedURLException;
-import java.net.SocketTimeoutException;
-import java.net.URL;
-import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -47,7 +43,6 @@ import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Stream;
-import java.util.concurrent.CompletableFuture;
 
 public class WebUtils {
 
@@ -111,11 +106,10 @@ public class WebUtils {
      *
      * @param url The URL to check
      * @param force If true, the pack will be downloaded even if it is cached to a separate location.
-     * @return Path to the downloaded pack file
+     * @return Path to the downloaded pack file, or null if it was unable to be loaded
      */
-    public static CompletableFuture<@Nullable Path> checkUrlAndDownloadRemotePack(String url, boolean force) {
+    public static @Nullable Path checkUrlAndDownloadRemotePack(String url, boolean force) {
         GeyserLogger logger = GeyserImpl.getInstance().getLogger();
-        return CompletableFuture.supplyAsync(() -> {
         try {
             HttpURLConnection con = (HttpURLConnection) new URL(url).openConnection();
 
@@ -126,16 +120,14 @@ public class WebUtils {
 
             int responseCode = con.getResponseCode();
             if (responseCode >= 400) {
-                    logger.error(String.format("Invalid response code from remote pack URL: %s (code: %d)", url, responseCode));
+                throw new IllegalStateException(String.format("Invalid response code from remote pack URL: %s (code: %d)", url, responseCode));
-                    return null;
             }
 
             int size = con.getContentLength();
             String type = con.getContentType();
 
             if (size <= 0) {
-                    logger.error(String.format("Invalid size from remote pack URL: %s (size: %d)", url, size));
+                throw new IllegalArgumentException(String.format("Invalid size from remote pack URL: %s (size: %d)", url, size));
-                    return null;
             }
 
             // This doesn't seem to be a requirement (anymore?). Logging to debug might be interesting though.
@@ -146,6 +138,7 @@ public class WebUtils {
             Path packLocation = REMOTE_PACK_CACHE.resolve(url.hashCode() + ".zip");
             Path packMetadata = packLocation.resolveSibling(url.hashCode() + ".metadata");
 
+            // If we downloaded this pack before, reuse it if the ETag matches.
             if (Files.exists(packLocation) && Files.exists(packMetadata) && !force) {
                 try {
                     List<String> metadataLines = Files.readAllLines(packMetadata, StandardCharsets.UTF_8);
@@ -166,17 +159,21 @@ public class WebUtils {
             Files.copy(con.getInputStream(), downloadLocation, StandardCopyOption.REPLACE_EXISTING);
 
             // This needs to match as the client fails to download the pack otherwise
-                if (Files.size(downloadLocation) != size) {
+            long downloadSize = Files.size(downloadLocation);
-                    GeyserImpl.getInstance().getLogger().error(String.format("Size mismatch with resource pack at url: %s. Downloaded pack has %s bytes, expected %s bytes!", url, Files.size(packLocation), size));
+            if (downloadSize != size) {
                 Files.delete(downloadLocation);
-                    return null;
+                throw new IllegalStateException("Size mismatch with resource pack at url: %s. Downloaded pack has %s bytes, expected %s bytes"
+                        .formatted(url, downloadSize, size));
             }
 
             // "Force" runs when the client rejected a pack. This is done for diagnosis of the issue.
             if (force) {
+                // Check whether existing pack's size matches the newly downloaded packs' size
                 if (Files.size(packLocation) != Files.size(downloadLocation)) {
-                        logger.error("The pack size seems to have changed. If you wish to change the pack at the remote URL, restart/reload Geyser. " +
+                    logger.error("""
-                                "Changing the pack mid-game can result in clients rejecting the pack, connected clients having different pack, or similar. ");
+                            The pack size seems to have changed (%s, expected %s). If you wish to change the pack at the remote URL, restart/reload Geyser.
+                            Changing the pack while Geyser is running can result in unexpected issues.
+                            """.formatted(Files.size(packLocation), Files.size(downloadLocation)));
                 }
             } else {
                 try {
@@ -188,14 +185,14 @@ public class WebUtils {
 
             return downloadLocation;
         } catch (MalformedURLException e) {
-                throw new IllegalArgumentException("Malformed URL: " + url);
+            throw new IllegalArgumentException("Unable to download resource pack from malformed URL %s! ".formatted(url));
         } catch (SocketTimeoutException | ConnectException e) {
-                GeyserImpl.getInstance().getLogger().error("Unable to reach URL: " + url + " (" + e.getMessage() + ")");
+            logger.error("Unable to download pack from url %s due to network error! ( %s )".formatted(url, e.getMessage()));
-                return null;
+            logger.debug(e);
         } catch (IOException e) {
-                throw new RuntimeException("Unable to download and save remote resource pack from: " + url + " (" + e.getMessage() + ")");
+            throw new IllegalStateException("Unable to download and save remote resource pack from: %s ( %s )!".formatted(url, e.getMessage()));
         }
-        });
+        return null;
     }