Make encode/decode error handling work correctly

2024-08-10 23:26:04 +05:00
parent ba3677b641
commit 606c157958
8 changed files with 42 additions and 22 deletions
@@ -52,11 +52,13 @@ public class ClientPluginChannelApi
 		Objects.requireNonNull(session);
 		this.session = session;
 		session.registerHandler(CurrentLevelKeyMessage.class, this::onCurrentLevelKeyMessage);
-		session.registerHandler(CloseEvent.class, false, this::onClose);
+		session.registerHandler(CloseEvent.class, this::onClose);
 	}
 	
 	private void onCurrentLevelKeyMessage(CurrentLevelKeyMessage msg)
 	{
+		// prefix@namespace:path
+		// 1-50 characters in total, all parts except namespace can be omitted
 		if (!msg.levelKey.matches("^(?=.{1,50}$)([a-zA-Z0-9-_]+@)?[a-zA-Z0-9-_]+(:[a-zA-Z0-9-_]+)?$"))
 		{
 			throw new IllegalArgumentException("Server sent invalid level key.");
@@ -64,7 +64,7 @@ public class ClientNetworkState implements Closeable
 			this.configReceived = true;
 		});
 		
-		this.session.registerHandler(CloseEvent.class, false, msg ->
+		this.session.registerHandler(CloseEvent.class, msg ->
 		{
 			this.configReceived = false;
 		});
@@ -53,8 +53,8 @@ public class ServerPlayerState
 			this.session.sendMessage(new RemotePlayerConfigMessage(this.config));
 		});
 		
-		this.session.registerHandler(CloseEvent.class, false, event -> {
-			// Noop
+		this.session.registerHandler(CloseEvent.class, event -> {
+			// No-op. removes "Unhandled message" log entries
 		});
 	}
 	
@@ -108,16 +108,11 @@ public abstract class NetworkEventSource
 		}
 	}
 	
-	public abstract <T extends NetworkMessage> void registerHandler(Class<T> handlerClass, boolean throwIfMessageNotRegistered, Consumer<T> handlerImplementation);
-
-	public final <T extends NetworkMessage> void registerHandler(Class<T> handlerClass, Consumer<T> handlerImplementation)
+	public abstract <T extends NetworkMessage> void registerHandler(Class<T> handlerClass, Consumer<T> handlerImplementation);
+	
+	protected final <T extends NetworkMessage> void registerHandler(NetworkEventSource instance, Class<T> handlerClass, Consumer<T> handlerImplementation)
 	{
-		this.registerHandler(handlerClass, true, handlerImplementation);
-	}
-
-	protected final <T extends NetworkMessage> void registerHandler(NetworkEventSource instance, Class<T> handlerClass, boolean throwIfMessageNotRegistered, Consumer<T> handlerImplementation)
-	{
-		if (throwIfMessageNotRegistered)
+		if (!InternalEvent.class.isAssignableFrom(handlerClass))
 		{
 			MessageRegistry.INSTANCE.getMessageId(handlerClass);
 		}
@@ -8,14 +8,16 @@ import org.jetbrains.annotations.Nullable;
 */
 public class ProtocolErrorEvent extends InternalEvent
 {
-	public final Throwable throwable;
+	public final Throwable reason;
 	@Nullable
 	public final NetworkMessage message;
+	public final boolean replyWithCloseReason;
 	
-	public ProtocolErrorEvent(Throwable throwable, @Nullable NetworkMessage message)
+	public ProtocolErrorEvent(Throwable reason, @Nullable NetworkMessage message, boolean replyWithCloseReason)
 	{
-		this.throwable = throwable;
+		this.reason = reason;
 		this.message = message;
+		this.replyWithCloseReason = replyWithCloseReason;
 	}
 	
 }
@@ -39,7 +39,7 @@ public final class ScopedNetworkEventSource extends NetworkEventSource
 	
 	
 	@Override
-	public <T extends NetworkMessage> void registerHandler(Class<T> handlerClass, boolean throwIfMessageNotRegistered, Consumer<T> handlerImplementation)
+	public <T extends NetworkMessage> void registerHandler(Class<T> handlerClass, Consumer<T> handlerImplementation)
 	{
 		if (this.isClosed)
 		{
@@ -47,9 +47,9 @@ public final class ScopedNetworkEventSource extends NetworkEventSource
 		}
 		
 		//noinspection unchecked
-		this.parent.registerHandler(this, handlerClass, throwIfMessageNotRegistered, (Consumer<T>) this.actualHandleMessageStable);
+		this.parent.registerHandler(this, handlerClass, (Consumer<T>) this.actualHandleMessageStable);
 		
-		super.registerHandler(this, handlerClass, throwIfMessageNotRegistered, handlerImplementation);
+		super.registerHandler(this, handlerClass, handlerImplementation);
 	}
 	
 	
@@ -21,6 +21,7 @@ package com.seibel.distanthorizons.core.network.messages;

 import com.google.common.collect.BiMap;
 import com.google.common.collect.HashBiMap;
+import com.seibel.distanthorizons.core.network.messages.base.CodecCrashMessage;
 import com.seibel.distanthorizons.core.network.messages.base.CurrentLevelKeyMessage;
 import com.seibel.distanthorizons.core.network.messages.base.RemotePlayerConfigMessage;
 import com.seibel.distanthorizons.core.network.messages.fullData.FullDataChunkMessage;
@@ -30,6 +31,7 @@ import com.seibel.distanthorizons.core.network.messages.requests.ExceptionMessag
 import com.seibel.distanthorizons.core.network.messages.fullData.FullDataPartialUpdateMessage;
 import com.seibel.distanthorizons.core.network.messages.fullData.FullDataSourceRequestMessage;
 import com.seibel.distanthorizons.core.network.messages.fullData.FullDataSourceResponseMessage;
+import com.seibel.distanthorizons.coreapi.ModInfo;

 import java.util.HashMap;
 import java.util.Map;
@@ -37,6 +39,7 @@ import java.util.function.Supplier;

 public class MessageRegistry
 {
+	public static final boolean DEBUG_ENABLE_CODEC_CRASH_MESSAGE = ModInfo.IS_DEV_BUILD;
 	public static final MessageRegistry INSTANCE = new MessageRegistry();
 	
 	private final Map<Integer, Supplier<? extends NetworkMessage>> idToSupplier = new HashMap<>();
@@ -49,6 +52,7 @@ public class MessageRegistry
 		// Note: Messages must have parameterless constructors
 		
 		// When the communication is about to be stopped, either side can send this message
+		// There may be messages after this, but they should be ignored if it's possible
 		this.registerMessage(CloseReasonMessage.class, CloseReasonMessage::new);
 		
 		// Level keys
@@ -66,6 +70,12 @@ public class MessageRegistry
 		this.registerMessage(FullDataSourceResponseMessage.class, FullDataSourceResponseMessage::new);
 		this.registerMessage(FullDataPartialUpdateMessage.class, FullDataPartialUpdateMessage::new);
 		this.registerMessage(FullDataChunkMessage.class, FullDataChunkMessage::new);
+		
+		// Debug messages are always last, and not included into release builds.
+		if (DEBUG_ENABLE_CODEC_CRASH_MESSAGE)
+		{
+			this.registerMessage(CodecCrashMessage.class, CodecCrashMessage::new);
+		}
 	}
 	
 	
@@ -5,6 +5,7 @@ import com.seibel.distanthorizons.core.dependencyInjection.SingletonInjector;
 import com.seibel.distanthorizons.core.logging.ConfigBasedLogger;
 import com.seibel.distanthorizons.core.network.event.NetworkEventSource;
 import com.seibel.distanthorizons.core.network.event.CloseEvent;
+import com.seibel.distanthorizons.core.network.event.ProtocolErrorEvent;
 import com.seibel.distanthorizons.core.network.messages.NetworkMessage;
 import com.seibel.distanthorizons.core.network.messages.TrackableMessage;
 import com.seibel.distanthorizons.core.network.messages.base.CloseReasonMessage;
@@ -48,6 +49,16 @@ public class Session extends NetworkEventSource
 		{
 			this.close(new SessionClosedException(msg.reason));
 		});
+		
+		this.registerHandler(ProtocolErrorEvent.class, event ->
+		{
+			if (event.replyWithCloseReason)
+			{
+				this.sendMessage(new CloseReasonMessage("Internal error on other side"));
+			}
+			
+			this.close(event.reason);
+		});
 	}
 	
 	
@@ -68,20 +79,20 @@ public class Session extends NetworkEventSource
 		catch (Throwable e)
 		{
 			LOGGER.error("Failed to handle the message. New messages will be ignored.", e);
-			LOGGER.error("Message: " + message);
+			LOGGER.error("Message: {}", message);
 			this.close();
 		}
 	}
 	
 	@Override
-	public <T extends NetworkMessage> void registerHandler(Class<T> handlerClass, boolean throwIfMessageNotRegistered, Consumer<T> handlerImplementation)
+	public <T extends NetworkMessage> void registerHandler(Class<T> handlerClass, Consumer<T> handlerImplementation)
 	{
 		if (this.closeReason.get() != null)
 		{
 			return;
 		}
 		
-		this.registerHandler(this, handlerClass, throwIfMessageNotRegistered, handlerImplementation);
+		this.registerHandler(this, handlerClass, handlerImplementation);
 	}
 	
 	public <TResponse extends TrackableMessage> CompletableFuture<TResponse> sendRequest(TrackableMessage msg, Class<TResponse> responseClass)