From e502e0019f434813a540bbe89c872d441695d00d Mon Sep 17 00:00:00 2001 From: Tim203 Date: Tue, 27 Dec 2022 01:08:02 +0100 Subject: [PATCH 1/3] Use weak references for injected Netty channels --- .../inject/CommonPlatformInjector.java | 17 +++--- .../inject/spigot/SpigotInjector.java | 58 ++++++++++++++----- 2 files changed, 54 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/org/geysermc/floodgate/inject/CommonPlatformInjector.java b/core/src/main/java/org/geysermc/floodgate/inject/CommonPlatformInjector.java index 9025ca0d..95e0a339 100644 --- a/core/src/main/java/org/geysermc/floodgate/inject/CommonPlatformInjector.java +++ b/core/src/main/java/org/geysermc/floodgate/inject/CommonPlatformInjector.java @@ -26,27 +26,30 @@ package org.geysermc.floodgate.inject; import io.netty.channel.Channel; +import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import java.util.Set; -import lombok.AccessLevel; -import lombok.Getter; +import java.util.WeakHashMap; import org.geysermc.floodgate.api.inject.InjectorAddon; import org.geysermc.floodgate.api.inject.PlatformInjector; public abstract class CommonPlatformInjector implements PlatformInjector { - @Getter(AccessLevel.PROTECTED) - private final Set injectedClients = new HashSet<>(); + private final Map injectedClients = + Collections.synchronizedMap(new WeakHashMap<>()); private final Map, InjectorAddon> addons = new HashMap<>(); protected boolean addInjectedClient(Channel channel) { - return injectedClients.add(channel); + return injectedClients.put(channel, null) != null; } public boolean removeInjectedClient(Channel channel) { - return injectedClients.remove(channel); + return injectedClients.remove(channel) != null; + } + + public Set injectedClients() { + return injectedClients.keySet(); } @Override diff --git a/spigot/src/main/java/org/geysermc/floodgate/inject/spigot/SpigotInjector.java b/spigot/src/main/java/org/geysermc/floodgate/inject/spigot/SpigotInjector.java index 2a718b0a..42b1034e 100644 --- a/spigot/src/main/java/org/geysermc/floodgate/inject/spigot/SpigotInjector.java +++ b/spigot/src/main/java/org/geysermc/floodgate/inject/spigot/SpigotInjector.java @@ -126,30 +126,60 @@ public final class SpigotInjector extends CommonPlatformInjector { return; } - // remove injection from clients - for (Channel channel : getInjectedClients()) { - removeAddonsCall(channel); - } - getInjectedClients().clear(); - - // and change the list back to the original + // let's change the list back to the original first + // so that new connections are not handled through our custom list Object serverConnection = getServerConnection(); if (serverConnection != null) { Field field = ReflectionUtils.getField(serverConnection.getClass(), injectedFieldName); - List list = (List) ReflectionUtils.getValue(serverConnection, field); - if (list instanceof CustomList) { - CustomList customList = (CustomList) list; - ReflectionUtils.setValue(serverConnection, field, customList.getOriginalList()); - customList.clear(); - customList.addAll(list); + if (!findAndReplaceCustomList(serverConnection, field)) { + throw new IllegalStateException( + "Unable to remove all references of Floodgate! " + + "Reloading will very likely break Floodgate." + ); } } + // remove injection from clients + for (Channel channel : injectedClients()) { + removeAddonsCall(channel); + } + + //todo make sure that all references are removed from the channels, + // except from one AttributeKey with Floodgate player data which could be used + // after reloading + injected = false; } - public Object getServerConnection() { + /** + * Replaces all references of CustomList with the original list. + * It's entirely possible that e.g. the most recent channel map override isn't Floodgate's + */ + private boolean findAndReplaceCustomList(Object object, Field possibleListField) { + Object value = ReflectionUtils.getValue(object, possibleListField); + + if (value == null || value.getClass() == Object.class) { + return false; + } + + if (value instanceof CustomList) { + // all we have to do is replace the list with the original list. + // the original list is up-to-date, so we don't have to clear/add/whatever anything + CustomList customList = (CustomList) value; + ReflectionUtils.setValue(object, possibleListField, customList.getOriginalList()); + } + + boolean found = false; + for (Field field : value.getClass().getDeclaredFields()) { + // normally list types don't have a lot of fields, so let's just try all of them + found |= findAndReplaceCustomList(value, field); + } + + return found; + } + + private Object getServerConnection() { if (serverConnection != null) { return serverConnection; } From c84dd0cae507b1fb0205a7f4d19dab403aaa7f8a Mon Sep 17 00:00:00 2001 From: Tim203 Date: Tue, 27 Dec 2022 12:05:47 +0100 Subject: [PATCH 2/3] Use newSetFromMap --- .../floodgate/inject/CommonPlatformInjector.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/geysermc/floodgate/inject/CommonPlatformInjector.java b/core/src/main/java/org/geysermc/floodgate/inject/CommonPlatformInjector.java index 95e0a339..1547a2be 100644 --- a/core/src/main/java/org/geysermc/floodgate/inject/CommonPlatformInjector.java +++ b/core/src/main/java/org/geysermc/floodgate/inject/CommonPlatformInjector.java @@ -35,21 +35,21 @@ import org.geysermc.floodgate.api.inject.InjectorAddon; import org.geysermc.floodgate.api.inject.PlatformInjector; public abstract class CommonPlatformInjector implements PlatformInjector { - private final Map injectedClients = - Collections.synchronizedMap(new WeakHashMap<>()); + private final Set injectedClients = + Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<>())); private final Map, InjectorAddon> addons = new HashMap<>(); protected boolean addInjectedClient(Channel channel) { - return injectedClients.put(channel, null) != null; + return injectedClients.add(channel); } public boolean removeInjectedClient(Channel channel) { - return injectedClients.remove(channel) != null; + return injectedClients.remove(channel); } public Set injectedClients() { - return injectedClients.keySet(); + return injectedClients; } @Override From eb8989b4005bbfa4707a80f01461e69b75f3d5b8 Mon Sep 17 00:00:00 2001 From: Tim203 Date: Thu, 29 Dec 2022 00:31:48 +0100 Subject: [PATCH 3/3] Don't try to remove all injector references --- .../inject/spigot/SpigotInjector.java | 55 +++++++------------ .../module/SpigotPlatformModule.java | 7 +-- 2 files changed, 22 insertions(+), 40 deletions(-) diff --git a/spigot/src/main/java/org/geysermc/floodgate/inject/spigot/SpigotInjector.java b/spigot/src/main/java/org/geysermc/floodgate/inject/spigot/SpigotInjector.java index 42b1034e..17812624 100644 --- a/spigot/src/main/java/org/geysermc/floodgate/inject/spigot/SpigotInjector.java +++ b/spigot/src/main/java/org/geysermc/floodgate/inject/spigot/SpigotInjector.java @@ -25,6 +25,8 @@ package org.geysermc.floodgate.inject.spigot; +import com.google.inject.Inject; +import com.google.inject.Singleton; import io.netty.channel.Channel; import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelHandlerContext; @@ -36,13 +38,15 @@ import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.util.List; import lombok.Getter; -import lombok.RequiredArgsConstructor; +import org.geysermc.floodgate.api.logger.FloodgateLogger; import org.geysermc.floodgate.inject.CommonPlatformInjector; import org.geysermc.floodgate.util.ClassNames; import org.geysermc.floodgate.util.ReflectionUtils; -@RequiredArgsConstructor +@Singleton public final class SpigotInjector extends CommonPlatformInjector { + @Inject private FloodgateLogger logger; + private Object serverConnection; private String injectedFieldName; @@ -131,13 +135,23 @@ public final class SpigotInjector extends CommonPlatformInjector { Object serverConnection = getServerConnection(); if (serverConnection != null) { Field field = ReflectionUtils.getField(serverConnection.getClass(), injectedFieldName); + Object value = ReflectionUtils.getValue(serverConnection, field); - if (!findAndReplaceCustomList(serverConnection, field)) { - throw new IllegalStateException( - "Unable to remove all references of Floodgate! " + - "Reloading will very likely break Floodgate." - ); + if (value instanceof CustomList) { + // all we have to do is replace the list with the original list. + // the original list is up-to-date, so we don't have to clear/add/whatever anything + CustomList customList = (CustomList) value; + ReflectionUtils.setValue(serverConnection, field, customList.getOriginalList()); + return; } + + // we could replace all references of CustomList that are directly in 'value', but that + // only brings you so far. ProtocolLib for example stores the original value + // (which would be our CustomList e.g.) in a separate object + logger.debug( + "Unable to remove all references of Floodgate due to {}! ", + value.getClass().getName() + ); } // remove injection from clients @@ -152,33 +166,6 @@ public final class SpigotInjector extends CommonPlatformInjector { injected = false; } - /** - * Replaces all references of CustomList with the original list. - * It's entirely possible that e.g. the most recent channel map override isn't Floodgate's - */ - private boolean findAndReplaceCustomList(Object object, Field possibleListField) { - Object value = ReflectionUtils.getValue(object, possibleListField); - - if (value == null || value.getClass() == Object.class) { - return false; - } - - if (value instanceof CustomList) { - // all we have to do is replace the list with the original list. - // the original list is up-to-date, so we don't have to clear/add/whatever anything - CustomList customList = (CustomList) value; - ReflectionUtils.setValue(object, possibleListField, customList.getOriginalList()); - } - - boolean found = false; - for (Field field : value.getClass().getDeclaredFields()) { - // normally list types don't have a lot of fields, so let's just try all of them - found |= findAndReplaceCustomList(value, field); - } - - return found; - } - private Object getServerConnection() { if (serverConnection != null) { return serverConnection; diff --git a/spigot/src/main/java/org/geysermc/floodgate/module/SpigotPlatformModule.java b/spigot/src/main/java/org/geysermc/floodgate/module/SpigotPlatformModule.java index 1654badb..d809701f 100644 --- a/spigot/src/main/java/org/geysermc/floodgate/module/SpigotPlatformModule.java +++ b/spigot/src/main/java/org/geysermc/floodgate/module/SpigotPlatformModule.java @@ -62,6 +62,7 @@ public final class SpigotPlatformModule extends AbstractModule { @Override protected void configure() { bind(PlatformUtils.class).to(SpigotPlatformUtils.class); + bind(CommonPlatformInjector.class).to(SpigotInjector.class); bind(Logger.class).annotatedWith(Names.named("logger")).toInstance(plugin.getLogger()); bind(FloodgateLogger.class).to(JavaUtilFloodgateLogger.class); } @@ -96,12 +97,6 @@ public final class SpigotPlatformModule extends AbstractModule { DebugAddon / PlatformInjector */ - @Provides - @Singleton - public CommonPlatformInjector platformInjector() { - return new SpigotInjector(); - } - @Provides @Named("packetEncoder") public String packetEncoder() {