From 4067107b52534b6fa1140e5734dd40dcaeab06a7 Mon Sep 17 00:00:00 2001 From: Nassim Jahnke Date: Fri, 6 Jan 2023 20:33:17 +0100 Subject: [PATCH] Improve exception handling --- .../api/protocol/AbstractProtocol.java | 6 ++-- .../exception/InformativeException.java | 9 ++++++ .../viaversion/util/PipelineUtil.java | 20 ++++++++++++ .../bukkit/handlers/BukkitDecodeHandler.java | 14 +++++--- .../bukkit/handlers/BukkitEncodeHandler.java | 14 +++++--- .../protocol/packet/PacketWrapperImpl.java | 32 ++++++++++++------- 6 files changed, 72 insertions(+), 23 deletions(-) diff --git a/api/src/main/java/com/viaversion/viaversion/api/protocol/AbstractProtocol.java b/api/src/main/java/com/viaversion/viaversion/api/protocol/AbstractProtocol.java index 8cb745213..42a525dca 100644 --- a/api/src/main/java/com/viaversion/viaversion/api/protocol/AbstractProtocol.java +++ b/api/src/main/java/com/viaversion/viaversion/api/protocol/AbstractProtocol.java @@ -334,8 +334,9 @@ public abstract class AbstractProtocol T get(Class objectClass) { + //noinspection unchecked return (T) storedObjects.get(objectClass); } diff --git a/api/src/main/java/com/viaversion/viaversion/exception/InformativeException.java b/api/src/main/java/com/viaversion/viaversion/exception/InformativeException.java index 506a58e10..4a86f50d7 100644 --- a/api/src/main/java/com/viaversion/viaversion/exception/InformativeException.java +++ b/api/src/main/java/com/viaversion/viaversion/exception/InformativeException.java @@ -27,6 +27,7 @@ import java.util.Map; public class InformativeException extends Exception { private final Map info = new HashMap<>(); + private boolean shouldBePrinted = true; private int sources; public InformativeException(Throwable cause) { @@ -46,6 +47,14 @@ public class InformativeException extends Exception { return sourceClazz.isAnonymousClass() ? sourceClazz.getName() + " (Anonymous)" : sourceClazz.getName(); } + public boolean shouldBePrinted() { + return shouldBePrinted; + } + + public void setShouldBePrinted(final boolean shouldBePrinted) { + this.shouldBePrinted = shouldBePrinted; + } + @Override public String getMessage() { StringBuilder builder = new StringBuilder("Please report this on the Via support Discord or open an issue on the relevant GitHub repository\n"); diff --git a/api/src/main/java/com/viaversion/viaversion/util/PipelineUtil.java b/api/src/main/java/com/viaversion/viaversion/util/PipelineUtil.java index c1344d9ac..c277163a3 100644 --- a/api/src/main/java/com/viaversion/viaversion/util/PipelineUtil.java +++ b/api/src/main/java/com/viaversion/viaversion/util/PipelineUtil.java @@ -28,6 +28,7 @@ import io.netty.channel.ChannelPipeline; import io.netty.handler.codec.ByteToMessageDecoder; import io.netty.handler.codec.MessageToByteEncoder; import io.netty.handler.codec.MessageToMessageDecoder; +import org.checkerframework.checker.nullness.qual.Nullable; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -116,6 +117,25 @@ public final class PipelineUtil { return false; } + /** + * Check if a stack trace contains a certain exception and returns it if present. + * + * @param t throwable + * @param c the exception to look for + * @return contained exception of t if present + */ + public static @Nullable T getCause(Throwable t, Class c) { + while (t != null) { + if (c.isAssignableFrom(t.getClass())) { + //noinspection unchecked + return (T) t; + } + + t = t.getCause(); + } + return null; + } + /** * Get the context for a the channel handler before a certain name. * diff --git a/bukkit/src/main/java/com/viaversion/viaversion/bukkit/handlers/BukkitDecodeHandler.java b/bukkit/src/main/java/com/viaversion/viaversion/bukkit/handlers/BukkitDecodeHandler.java index 42f6c5546..6d2eb2062 100644 --- a/bukkit/src/main/java/com/viaversion/viaversion/bukkit/handlers/BukkitDecodeHandler.java +++ b/bukkit/src/main/java/com/viaversion/viaversion/bukkit/handlers/BukkitDecodeHandler.java @@ -17,9 +17,7 @@ */ package com.viaversion.viaversion.bukkit.handlers; -import com.viaversion.viaversion.api.Via; import com.viaversion.viaversion.api.connection.UserConnection; -import com.viaversion.viaversion.api.protocol.packet.State; import com.viaversion.viaversion.bukkit.util.NMSUtil; import com.viaversion.viaversion.exception.CancelCodecException; import com.viaversion.viaversion.exception.CancelDecoderException; @@ -67,9 +65,15 @@ public final class BukkitDecodeHandler extends MessageToMessageDecoder } super.exceptionCaught(ctx, cause); - if (!NMSUtil.isDebugPropertySet() && PipelineUtil.containsCause(cause, InformativeException.class) - && (connection.getProtocolInfo().getState() != State.HANDSHAKE || Via.getManager().isDebug())) { - cause.printStackTrace(); // Print if CB doesn't already do it + if (NMSUtil.isDebugPropertySet()) { + return; + } + + // Print if CB doesn't already do it + final InformativeException exception = PipelineUtil.getCause(cause, InformativeException.class); + if (exception != null && exception.shouldBePrinted()) { + cause.printStackTrace(); + exception.setShouldBePrinted(false); } } diff --git a/bukkit/src/main/java/com/viaversion/viaversion/bukkit/handlers/BukkitEncodeHandler.java b/bukkit/src/main/java/com/viaversion/viaversion/bukkit/handlers/BukkitEncodeHandler.java index 163f7d433..b58898a2f 100644 --- a/bukkit/src/main/java/com/viaversion/viaversion/bukkit/handlers/BukkitEncodeHandler.java +++ b/bukkit/src/main/java/com/viaversion/viaversion/bukkit/handlers/BukkitEncodeHandler.java @@ -17,9 +17,7 @@ */ package com.viaversion.viaversion.bukkit.handlers; -import com.viaversion.viaversion.api.Via; import com.viaversion.viaversion.api.connection.UserConnection; -import com.viaversion.viaversion.api.protocol.packet.State; import com.viaversion.viaversion.bukkit.util.NMSUtil; import com.viaversion.viaversion.exception.CancelCodecException; import com.viaversion.viaversion.exception.CancelEncoderException; @@ -110,9 +108,15 @@ public final class BukkitEncodeHandler extends MessageToMessageEncoder } super.exceptionCaught(ctx, cause); - if (!NMSUtil.isDebugPropertySet() && PipelineUtil.containsCause(cause, InformativeException.class) - && (connection.getProtocolInfo().getState() != State.HANDSHAKE || Via.getManager().isDebug())) { - cause.printStackTrace(); // Print if CB doesn't already do it + if (NMSUtil.isDebugPropertySet()) { + return; + } + + // Print if CB doesn't already do it + final InformativeException exception = PipelineUtil.getCause(cause, InformativeException.class); + if (exception != null && exception.shouldBePrinted()) { + cause.printStackTrace(); + exception.setShouldBePrinted(false); } } diff --git a/common/src/main/java/com/viaversion/viaversion/protocol/packet/PacketWrapperImpl.java b/common/src/main/java/com/viaversion/viaversion/protocol/packet/PacketWrapperImpl.java index 5eb371e58..9e3665675 100644 --- a/common/src/main/java/com/viaversion/viaversion/protocol/packet/PacketWrapperImpl.java +++ b/common/src/main/java/com/viaversion/viaversion/protocol/packet/PacketWrapperImpl.java @@ -49,7 +49,9 @@ public class PacketWrapperImpl implements PacketWrapper { private final ByteBuf inputBuffer; private final UserConnection userConnection; private boolean send = true; - /** Only non-null if specifically set and gotten before packet transformation */ + /** + * Only non-null if specifically set and gotten before packet transformation + */ private PacketType packetType; private int id; private final Deque> readableObjects = new ArrayDeque<>(); @@ -78,9 +80,7 @@ public class PacketWrapperImpl implements PacketWrapper { } currentIndex++; } - - Exception e = new ArrayIndexOutOfBoundsException("Could not find type " + type.getTypeName() + " at " + index); - throw new InformativeException(e).set("Type", type.getTypeName()).set("Index", index).set("Packet ID", getId()).set("Packet Type", packetType).set("Data", packetValues); + throw createInformativeException(new ArrayIndexOutOfBoundsException("Could not find type " + type.getTypeName() + " at " + index), type, index); } @Override @@ -121,20 +121,22 @@ public class PacketWrapperImpl implements PacketWrapper { } currentIndex++; } - Exception e = new ArrayIndexOutOfBoundsException("Could not find type " + type.getTypeName() + " at " + index); - throw new InformativeException(e).set("Type", type.getTypeName()).set("Index", index).set("Packet ID", getId()).set("Packet Type", packetType); + throw createInformativeException(new ArrayIndexOutOfBoundsException("Could not find type " + type.getTypeName() + " at " + index), type, index); } @Override public T read(Type type) throws Exception { - if (type == Type.NOTHING) return null; + if (type == Type.NOTHING) { + return null; + } + if (readableObjects.isEmpty()) { Preconditions.checkNotNull(inputBuffer, "This packet does not have an input buffer."); // We could in the future log input read values, but honestly for things like bulk maps, mem waste D: try { return type.read(inputBuffer); } catch (Exception e) { - throw new InformativeException(e).set("Type", type.getTypeName()).set("Packet ID", getId()).set("Packet Type", packetType).set("Data", packetValues); + throw createInformativeException(e, type, packetValues.size() + 1); } } @@ -147,8 +149,7 @@ public class PacketWrapperImpl implements PacketWrapper { } else if (rtype == Type.NOTHING) { return read(type); // retry } else { - Exception e = new IOException("Unable to read type " + type.getTypeName() + ", found " + read.key().getTypeName()); - throw new InformativeException(e).set("Type", type.getTypeName()).set("Packet ID", getId()).set("Packet Type", packetType).set("Data", packetValues); + throw createInformativeException(new IOException("Unable to read type " + type.getTypeName() + ", found " + read.key().getTypeName()), type, readableObjects.size()); } } @@ -209,13 +210,22 @@ public class PacketWrapperImpl implements PacketWrapper { try { packetValue.key().write(buffer, packetValue.value()); } catch (Exception e) { - throw new InformativeException(e).set("Index", index).set("Type", packetValue.key().getTypeName()).set("Packet ID", getId()).set("Packet Type", packetType).set("Data", packetValues); + throw createInformativeException(e, packetValue.key(), index); } index++; } writeRemaining(buffer); } + private InformativeException createInformativeException(final Exception cause, final Type type, final int index) { + return new InformativeException(cause) + .set("Index", index) + .set("Type", type.getTypeName()) + .set("Packet ID", this.id) + .set("Packet Type", this.packetType) + .set("Data", this.packetValues); + } + @Override public void clearInputBuffer() { if (inputBuffer != null) {