mcgrizzz / Proton

MIT License
16 stars 3 forks source link

RegisterMessageHandlerException caused by plugman reload #35

Closed LogGits closed 3 years ago

LogGits commented 3 years ago

Getting this error when reloading my plugin via plugman.

https://github.com/mcgrizzz/Proton/blob/0be53380b08844e25066cc59f8d9f1171f7c3c0d/src/main/java/me/drepic/proton/common/ProtonManager.java#L232

On startup im registering a Bukkit Listener class and inside that class which is initialised using:

plugin.getManager().registerMessageHandlers(this)

From what it looks like my plugin is starting, registering the class, then when I plugman reload it's attempting to register the class again. How can I handle this gracefully (via plugin reload/shutdown) so I can use Proton while testing?

Cheers in advance. Loving this system, keep up the great work :)

P.s. I know it's not good to use plugman to reload the plugin but when im testing it's convenient.

mcgrizzz commented 3 years ago

Hey, sorry for some reason I wasn't notified of this issue.

Right now there is no logic to unregister handlers. I suppose I can add a line to clear them onDisable which should resolve those issues.

When I release it, please let me know if you have any issues.

LogGits commented 3 years ago

Ah ok, in this usecase idk if this would solve the problem because proton wouldnt be getting reloaded my plugin that utilises proton would be. I think id need to unregister them from within my plugin ondisable.

mcgrizzz commented 3 years ago

Actually, now that I see the error, it seems the issue is not that you're reloading. That error should only be thrown if the datatype is changing for the same message context.

Do you happen to be changing the class you're trying to send before each reload? If that's the case, even if the class name is the same, if the class members are different java will see it as a different class.

I can look into writing an unregister method so you can manually override any existing mappings, although at the moment I don't know how that will look.

EDIT: An alternate idea is that I implement a register method which takes in a boolean as to whether or not the mappings should be overridden (in that case, that specific error would never be thrown, maybe a warning). Although that could make debugging in some cases confusing.

EDIT 2:

I am hesitant about this because it would essentially allow anyone to hijack other plugins using the API given you know the MessageContext. Perhaps there's another solution.

mcgrizzz commented 3 years ago

I'm curious, does Plugman reload all the dependants of whatever you reload. For example, if you were to reload Proton, would it also then reload your plugin?

In that case, I think simply clearing the registration onDisable would work.

Sorry for the message spam btw.

LogGits commented 3 years ago

Interesting find, ill review the me code tomorrow (its 4am rn). It should be registering the same class but could something like a different variable assignment (e.g. say there was a long storing time of initialisation)?

I'm curious, does Plugman reload all the dependants of whatever you reload. For example, if you were to reload Proton, would it also then reload your plugin?

In that case, I think simply clearing the registration onDisable would work.

Sorry for the message spam btw.

I think it only reloads the specific plugin.

mcgrizzz commented 3 years ago

Interesting find, ill review the me code tomorrow (its 4am rn). It should be registering the same class but could something like a different variable assignment (e.g. say there was a long storing time of initialisation)?

I'm curious, does Plugman reload all the dependants of whatever you reload. For example, if you were to reload Proton, would it also then reload your plugin? In that case, I think simply clearing the registration onDisable would work. Sorry for the message spam btw.

I think it only reloads the specific plugin.

What I mean is that if for example you have a class: DataHolder.class

At first you have something like this:

public class DataHolder{

     int data;

}

However, you change it to have another class member:

public class DataHolder{

     int data;
     int data2;

}

These are technically different classes, even with the same name. I think the same occurs with a change to methods/constructors but I haven't fully tested it.

LogGits commented 3 years ago

Class is registered in constructor like:

public class PlayerChat implements Listener {
    public PlayerChat()
    {
        Main.getManager().registerMessageHandlers(this)
    }
    @EventHandler
    public void onQuit()
    {
        //etc
    }
    @MessageHandler (namespace = "backend", subject = "command")
    public void receivedCommand (String command) {
        Methods.runCommand(command);
    }
}

The class is called inside the onEnable via:

getServer().getPluginManager().registerEvents(new PlayerChat(), this);

I just changed the constructor to take in a JavaPlugin arg instead of using the Main class, Will test if that works later tonight.

mcgrizzz commented 3 years ago

That shouldn't throw this error.

The class that is important is the parameter in your receiveCommand method. (In this case a String) Make sure you're not using the same namespace and subject somewhere else where you expect a different datatype.

LogGits commented 3 years ago

@mcgrizzz Sorry for the late response, I've been really busy this week. On the spigot plugin I have:

    @MessageHandler (namespace = "backend", subject = "command")
    public void receivedCommand (String command) {
        Methods.runCommand(command);
    }

On the proxy plugin i have:

    @MessageHandler (namespace = "backend", subject = "command")
    public void onCommand (String command) {
        Methods.runCommand(command);
    }

I utilise the .send with recipient set to the server it's meant to goto. This way i can have a backend highway for commands with recipients used to determine which server the command gets sent to. Could this be where the issue is? If so, how can we solve this (i'd like to keep them in the same namespace & channel if possible)

mcgrizzz commented 3 years ago

I don't see any issue with the code you're providing, They're both the same datatype. Next time you see the error can you send me the stack trace?

LogGits commented 3 years ago

Made some pretty significant changes but it still has the issue when I use /plugman reload {myPlugin}.

Console08:53:55
me.drepic.proton.common.exception.RegisterMessageHandlerException: MessageContext already has defined data type
at me.drepic.proton.common.ProtonManager.registerMessageHandler(ProtonManager.java:232) ~[?:?]
at me.drepic.proton.common.ProtonManager.registerMessageHandlers(ProtonManager.java:162) ~[?:?]
at me.loggits.testsuite.Main.onEnable(Main.java:112) ~[?:?]
at org.bukkit.plugin.java.JavaPlugin.setEnabled(JavaPlugin.java:263) ~[patched_1.16.5.jar:git-Paper-634]
at org.bukkit.plugin.java.JavaPluginLoader.enablePlugin(JavaPluginLoader.java:380) ~[patched_1.16.5.jar:git-Paper-634]
at org.bukkit.plugin.SimplePluginManager.enablePlugin(SimplePluginManager.java:483) ~[patched_1.16.5.jar:git-Paper-634]
at com.rylinaux.plugman.util.PluginUtil.load(PluginUtil.java:366) ~[?:?]
at com.rylinaux.plugman.util.PluginUtil.load(PluginUtil.java:318) ~[?:?]
at com.rylinaux.plugman.util.PluginUtil.reload(PluginUtil.java:380) ~[?:?]
at com.rylinaux.plugman.command.ReloadCommand.execute(ReloadCommand.java:122) ~[?:?]
at com.rylinaux.plugman.PlugManCommandHandler.onCommand(PlugManCommandHandler.java:95) ~[?:?]
at org.bukkit.command.PluginCommand.execute(PluginCommand.java:45) ~[patched_1.16.5.jar:git-Paper-634]
at org.bukkit.command.SimpleCommandMap.dispatch(SimpleCommandMap.java:159) ~[patched_1.16.5.jar:git-Paper-634]
at org.bukkit.craftbukkit.v1_16_R3.CraftServer.dispatchCommand(CraftServer.java:810) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.PlayerConnection.handleCommand(PlayerConnection.java:2171) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.PlayerConnection.c(PlayerConnection.java:1986) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.PlayerConnection.a(PlayerConnection.java:1939) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.PacketPlayInChat.a(PacketPlayInChat.java:50) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.PacketPlayInChat.a(PacketPlayInChat.java:8) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.PlayerConnectionUtils.lambda$ensureMainThread$1(PlayerConnectionUtils.java:35) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.TickTask.run(SourceFile:18) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.IAsyncTaskHandler.executeTask(IAsyncTaskHandler.java:136) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.IAsyncTaskHandlerReentrant.executeTask(SourceFile:23) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.IAsyncTaskHandler.executeNext(IAsyncTaskHandler.java:109) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.MinecraftServer.bb(MinecraftServer.java:1262) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.MinecraftServer.executeNext(MinecraftServer.java:1255) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.IAsyncTaskHandler.awaitTasks(IAsyncTaskHandler.java:119) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.MinecraftServer.sleepForTick(MinecraftServer.java:1216) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.MinecraftServer.w(MinecraftServer.java:1130) ~[patched_1.16.5.jar:git-Paper-634]
at net.minecraft.server.v1_16_R3.MinecraftServer.lambda$a$0(MinecraftServer.java:289) ~[patched_1.16.5.jar:git-Paper-634]
at java.lang.Thread.run(Thread.java:834) [?:?]

Registering via onEnable now, using:

manager = ProtonProvider.get();
manager.registerMessageHandlers(new PlayerChat(this));

PlayerChat:

public class PlayerChat {
    BaseComponent[] test;
    BaseComponent[] test2;
    Main plugin;

    public PlayerChat(Main plugin) {
        this.plugin = plugin;
        test = Text.createRunText(
                "You can enter the area using the bridge.",
                "&eClick here for more info.",
                "/info"
        );
        test2 = Text.createRunText(
                "You can exit the area using the side portal.",
                "&eClick here for more info.",
                "/info 2"
        );
    }

    @MessageHandler(namespace = "backend", subject = "command")
    public void onCommand(String command) {
        Methods.runCommand(command);
    }

    @MessageHandler(namespace = "backend", subject = "perm")
    public void onRequest(RequestData request){
        //...
    }

    @MessageHandler(namespace = "backend", subject = "swear")
    public void onDataArrive(Data data) {
        Bukkit.getScheduler().runTaskAsynchronously(plugin, () -> {
            Hyper.runTasks(data);
        });
    }

}

these are the namespaces:

Spigot:
@MessageHandler (namespace = "backend", subject = "swear")
@MessageHandler (namespace = "backend", subject = "perm")
@MessageHandler (namespace = "backend", subject = "command") // same signature as proxy

Proxy:
@MessageHandler (namespace = "discord", subject = "msg")
@MessageHandler (namespace = "backend", subject = "action")
@MessageHandler (namespace = "backend", subject = "verify")
@MessageHandler (namespace = "backend", subject = "command") // same signature as spigot
@MessageHandler (namespace = "discord", subject = "announcement")
@MessageHandler (namespace = "discord", subject = "rankdata")
@MessageHandler (namespace = "backend", subject = "purge")

If you're able to test, can you try using Plugman, Proton, and a Proton Plugin and use the cmd /plugman reload {protonpluginname}(reload plugin that relies on Proton). This should reproduce the error (though I'm not 100% certain because alot of issues I have are never reproducible haha)

mcgrizzz commented 3 years ago

EDIT: Ignore this for now:

So I was unable to reproduce this error on my side using my Dynamic Proxy plugin. Are you using this version of Plugman on spigot or the older version? If that is not the issue I think there is something about your code that I'm still not seeing. You can send me your code privately if you do not want to publically share all your code. But I think at this point it'll be hard for me to pinpoint the issue without having access to that. Ideally, I'd like to be able to run the code and debug it.

Another option is that you can methodically comment out your message listeners and test to see which one specifically is causing the issue and we can go from there.

Try reloading Proton before you reload your plugin. That should clear any mappings that are in memory and there shouldn't be issues then when you reload your plugin. I will say though that this should only be done when developing.

On another note, you can tell Proton that you want it to receive messages asynchronously:

@MessageHandler(namespace = "backend", subject = "swear")
  public void onDataArrive(Data data) {
      Bukkit.getScheduler().runTaskAsynchronously(plugin, () -> {
       Hyper.runTasks(data);
      });
  }

Can become:

@MessageHandler(namespace = "backend", subject = "swear", async = true)
  public void onDataArrive(Data data) {
      Hyper.runTasks(data);
  }
mcgrizzz commented 3 years ago

Closing this for now, if you have any other questions/replies you should be able to reopen.