From 86f645899f005ff01a6d4057b43cc4283ac83530 Mon Sep 17 00:00:00 2001 From: onebeastchris Date: Wed, 19 Jun 2024 23:35:36 +0200 Subject: [PATCH] Code cleanup, less futures, more exceptions when needed --- .../geysermc/geyser/api/pack/PackCodec.java | 2 +- .../geyser/api/pack/UrlPackCodec.java | 2 - .../java/org/geysermc/geyser/GeyserImpl.java | 2 - .../geyser/network/UpstreamPacketHandler.java | 2 +- .../geyser/pack/url/GeyserUrlPackCodec.java | 48 ++--- .../registry/loader/ResourcePackLoader.java | 97 +++++----- .../org/geysermc/geyser/util/WebUtils.java | 169 +++++++++--------- 7 files changed, 155 insertions(+), 167 deletions(-) diff --git a/api/src/main/java/org/geysermc/geyser/api/pack/PackCodec.java b/api/src/main/java/org/geysermc/geyser/api/pack/PackCodec.java index 7c123b624..ba062535d 100644 --- a/api/src/main/java/org/geysermc/geyser/api/pack/PackCodec.java +++ b/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. diff --git a/api/src/main/java/org/geysermc/geyser/api/pack/UrlPackCodec.java b/api/src/main/java/org/geysermc/geyser/api/pack/UrlPackCodec.java index ee449aa7f..8f279ae0d 100644 --- a/api/src/main/java/org/geysermc/geyser/api/pack/UrlPackCodec.java +++ b/api/src/main/java/org/geysermc/geyser/api/pack/UrlPackCodec.java @@ -36,8 +36,6 @@ import org.checkerframework.checker.nullness.qual.NonNull; *
  • be a direct download link to a .zip or .mcpack resource pack
    • *
  • use the application type `application/zip` and set a correct content length
    • * - * - * 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 { diff --git a/core/src/main/java/org/geysermc/geyser/GeyserImpl.java b/core/src/main/java/org/geysermc/geyser/GeyserImpl.java index cc31de9c5..0d47560b8 100644 --- a/core/src/main/java/org/geysermc/geyser/GeyserImpl.java +++ b/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); diff --git a/core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java b/core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java index 5061a7ef2..1f1d69136 100644 --- a/core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java +++ b/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()); diff --git a/core/src/main/java/org/geysermc/geyser/pack/url/GeyserUrlPackCodec.java b/core/src/main/java/org/geysermc/geyser/pack/url/GeyserUrlPackCodec.java index 0b0ce32e3..2c561fe1a 100644 --- a/core/src/main/java/org/geysermc/geyser/pack/url/GeyserUrlPackCodec.java +++ b/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 String contentKey; + private final @NonNull String url; + 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 - if (url == null) { - throw new IllegalArgumentException("Url cannot be nulL!"); - } + Objects.requireNonNull(url, "url cannot be null"); this.url = url; - this.contentKey = contentKey == null ? "" : contentKey; + this.contentKey = contentKey; } @Override public byte @NonNull [] sha256() { - if (this.fallback == null) { - throw new IllegalStateException("Fallback pack not initialized! Needs to be created first."); - } + Objects.requireNonNull(fallback, "must call #create() before attempting to get the sha256!"); return fallback.sha256(); } @Override public long size() { - if (this.fallback == null) { - throw new IllegalStateException("Fallback pack not initialized! Needs to be created first."); - } + Objects.requireNonNull(fallback, "must call #create() before attempting to get the size!"); return fallback.size(); } @Override public @NonNull SeekableByteChannel serialize(@NonNull ResourcePack resourcePack) throws IOException { - if (this.fallback == null) { - throw new IllegalStateException("Fallback pack not initialized! Needs to be created first."); - } + Objects.requireNonNull(fallback, "must call #create() before attempting to serialize!!"); 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()); - if (GeyserImpl.getInstance().getConfig().isDebugMode()) { - GeyserImpl.getInstance().getLogger().error("full error: " + throwable); - } + throw new IllegalArgumentException(throwable); + } else if (pack != null) { + 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 : ""; } } diff --git a/core/src/main/java/org/geysermc/geyser/registry/loader/ResourcePackLoader.java b/core/src/main/java/org/geysermc/geyser/registry/loader/ResourcePackLoader.java index 97ed256f9..2e8fe49cf 100644 --- a/core/src/main/java/org/geysermc/geyser/registry/loader/ResourcePackLoader.java +++ b/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.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; 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 brokenPacks = new HashSet<>(); + private static final Cache 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 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); @@ -231,7 +230,7 @@ public class ResourcePackLoader implements RegistryLoader remotePackUrls = GeyserImpl.getInstance().getConfig().getResourcePackUrls(); Map packMap = new Object2ObjectOpenHashMap<>(); - for (String url: remotePackUrls) { + for (String url : remotePackUrls) { try { GeyserUrlPackCodec codec = new GeyserUrlPackCodec(url); ResourcePack pack = codec.create(); @@ -246,57 +245,65 @@ public class ResourcePackLoader implements RegistryLoader downloadPack(String url, boolean checking) throws IllegalArgumentException { - return WebUtils.checkUrlAndDownloadRemotePack(url, checking).whenCompleteAsync((cachedPath, throwable) -> { - if (cachedPath == null) { - // already warned about in WebUtils - return; - } + public static CompletableFuture<@Nullable PathPackCodec> downloadPack(String url, boolean testing) throws IllegalArgumentException { + return CompletableFuture.supplyAsync(() -> { + Path path = WebUtils.checkUrlAndDownloadRemotePack(url, testing); - if (throwable != null) { - GeyserImpl.getInstance().getLogger().error("Failed to download resource pack! ", throwable); - return; + // Already warned about these above + if (path == null) { + 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); - } catch (IOException e) { - throw new IllegalArgumentException("Could not delete debug pack! " + e.getMessage(), e); - } - } - try { - try (ZipFile zip = new ZipFile(cachedPath.toFile())) { + try (ZipFile zip = new ZipFile(path.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. -// // (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) { -// GeyserImpl.getInstance().getLogger().debug("The remote resource pack from " + url + " contains a manifest.json file at the root of the zip file. " + -// "This is not supported for remote packs, and will 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."); -// } + // 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) + if (zip.getEntry("manifest.json") != null || zip.getEntry("pack_manifest.json") != null) { + if (testing) { + GeyserImpl.getInstance().getLogger().info("The remote resource pack from " + url + " contains a manifest.json file at the root of the zip file. " + + "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 { + 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 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(); + try { + HttpURLConnection con = (HttpURLConnection) new URL(url).openConnection(); - con.setConnectTimeout(10000); - con.setReadTimeout(10000); - con.setRequestProperty("User-Agent", "Geyser-" + GeyserImpl.getInstance().getPlatformType().platformName() + "/" + GeyserImpl.VERSION); - con.setInstanceFollowRedirects(true); + con.setConnectTimeout(10000); + con.setReadTimeout(10000); + con.setRequestProperty("User-Agent", "Geyser-" + GeyserImpl.getInstance().getPlatformType().platformName() + "/" + GeyserImpl.VERSION); + con.setInstanceFollowRedirects(true); - int responseCode = con.getResponseCode(); - if (responseCode >= 400) { - logger.error(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)); - return null; - } - - // This doesn't seem to be a requirement (anymore?). Logging to debug might be interesting though. - if (type == null || !type.equals("application/zip")) { - logger.debug(String.format("Application type from remote pack URL: %s (type: %s)", url, type)); - } - - Path packLocation = REMOTE_PACK_CACHE.resolve(url.hashCode() + ".zip"); - Path packMetadata = packLocation.resolveSibling(url.hashCode() + ".metadata"); - - if (Files.exists(packLocation) && Files.exists(packMetadata) && !force) { - try { - List metadataLines = Files.readAllLines(packMetadata, StandardCharsets.UTF_8); - int cachedSize = Integer.parseInt(metadataLines.get(0)); - String cachedEtag = metadataLines.get(1); - long cachedLastModified = Long.parseLong(metadataLines.get(2)); - - if (cachedSize == size && cachedEtag.equals(con.getHeaderField("ETag")) && cachedLastModified == con.getLastModified()) { - logger.debug("Using cached pack for " + url); - return packLocation; - } - } catch (IOException e) { - GeyserImpl.getInstance().getLogger().error("Failed to read cached pack metadata: " + e.getMessage()); - } - } - - Path downloadLocation = force ? REMOTE_PACK_CACHE.resolve(url.hashCode() + "_debug") : packLocation; - 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) { - 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)); - Files.delete(downloadLocation); - return null; - } - - // "Force" runs when the client rejected a pack. This is done for diagnosis of the issue. - if (force) { - 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. " + - "Changing the pack mid-game can result in clients rejecting the pack, connected clients having different pack, or similar. "); - } - } else { - try { - Files.write(packMetadata, Arrays.asList(String.valueOf(size), con.getHeaderField("ETag"), String.valueOf(con.getLastModified()))); - } catch (IOException e) { - GeyserImpl.getInstance().getLogger().error("Failed to write cached pack metadata: " + e.getMessage()); - } - } - - return downloadLocation; - } catch (MalformedURLException e) { - throw new IllegalArgumentException("Malformed URL: " + url); - } catch (SocketTimeoutException | ConnectException e) { - GeyserImpl.getInstance().getLogger().error("Unable to reach URL: " + url + " (" + e.getMessage() + ")"); - return null; - } catch (IOException e) { - throw new RuntimeException("Unable to download and save remote resource pack from: " + url + " (" + e.getMessage() + ")"); + int responseCode = con.getResponseCode(); + if (responseCode >= 400) { + throw new IllegalStateException(String.format("Invalid response code from remote pack URL: %s (code: %d)", url, responseCode)); } - }); + + int size = con.getContentLength(); + String type = con.getContentType(); + + if (size <= 0) { + throw new IllegalArgumentException(String.format("Invalid size from remote pack URL: %s (size: %d)", url, size)); + } + + // This doesn't seem to be a requirement (anymore?). Logging to debug might be interesting though. + if (type == null || !type.equals("application/zip")) { + logger.debug(String.format("Application type from remote pack URL: %s (type: %s)", url, type)); + } + + 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 metadataLines = Files.readAllLines(packMetadata, StandardCharsets.UTF_8); + int cachedSize = Integer.parseInt(metadataLines.get(0)); + String cachedEtag = metadataLines.get(1); + long cachedLastModified = Long.parseLong(metadataLines.get(2)); + + if (cachedSize == size && cachedEtag.equals(con.getHeaderField("ETag")) && cachedLastModified == con.getLastModified()) { + logger.debug("Using cached pack for " + url); + return packLocation; + } + } catch (IOException e) { + GeyserImpl.getInstance().getLogger().error("Failed to read cached pack metadata: " + e.getMessage()); + } + } + + Path downloadLocation = force ? REMOTE_PACK_CACHE.resolve(url.hashCode() + "_debug") : packLocation; + Files.copy(con.getInputStream(), downloadLocation, StandardCopyOption.REPLACE_EXISTING); + + // This needs to match as the client fails to download the pack otherwise + long downloadSize = Files.size(downloadLocation); + if (downloadSize != size) { + Files.delete(downloadLocation); + 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 (%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 { + Files.write(packMetadata, Arrays.asList(String.valueOf(size), con.getHeaderField("ETag"), String.valueOf(con.getLastModified()))); + } catch (IOException e) { + GeyserImpl.getInstance().getLogger().error("Failed to write cached pack metadata: " + e.getMessage()); + } + } + + return downloadLocation; + } catch (MalformedURLException e) { + throw new IllegalArgumentException("Unable to download resource pack from malformed URL %s! ".formatted(url)); + } catch (SocketTimeoutException | ConnectException e) { + logger.error("Unable to download pack from url %s due to network error! ( %s )".formatted(url, e.getMessage())); + logger.debug(e); + } catch (IOException e) { + throw new IllegalStateException("Unable to download and save remote resource pack from: %s ( %s )!".formatted(url, e.getMessage())); + } + return null; }