PaperMC / Velocity

The modern, next-generation Minecraft server proxy.
https://papermc.io/software/velocity
GNU General Public License v3.0
1.68k stars 583 forks source link

Event manager is registering something in listener that shouldn't be registered #1343

Open ham1255 opened 4 weeks ago

ham1255 commented 4 weeks ago

The problem

Since if i want multi platform support i tend to create API not to be depend on proxy that used which I create ITestEvent.java and ITestListener.java which then i implement in it proxy api which can be seen in steps to reproduce.

what i should expect that event to be handled single time but instead it fired 2 times, well this was weird so i checked if i didn't double register the listener or something, and it wasn't so i kept debugging until i reached idea it could be velocity problem, so after debugging with velocity, so IN VelocityEventManager at registerInternally after collectMethods i inserted this debug code

  public void registerInternally(final PluginContainer pluginContainer, final Object listener) {
    final Class<?> targetClass = listener.getClass();
    final Map<String, MethodHandlerInfo> collected = new HashMap<>();
    collectMethods(targetClass, collected);
    collected.forEach((clazz, coll) -> {
      logger.info("ID? {} | method {}", clazz, coll.method);
    });
    final List<HandlerRegistration> registrations = new ArrayList<>();

after adding debug messages i could see 2 methods registered

[18:26:39] [Velocity Async Event Executor - #0/INFO] [com.velocitypowered.proxy.event.VelocityEventManager]: ID? doSomething(net.limework.test.TestEvent): method public void net.limework.test.TestListener.doSomething(net.limework.test.TestEvent)
[18:26:39] [Velocity Async Event Executor - #0/INFO] [com.velocitypowered.proxy.event.VelocityEventManager]: ID? doSomething(net.limework.test.ITestEvent): method public void net.limework.test.TestListener.doSomething(net.limework.test.ITestEvent)

doSomething(net.limework.test.ITestEvent) shouldn't be registered since it does not have @Subscribe

steps to reproduce

  1. create classes in this type of configuration

public interface ITestEvent {

int x();

}

* TestEvent.java 
```java
package net.limework.test;

public record TestEvent(int x) implements ITestEvent {

}

public interface ITestListener {

void doSomething(E event);

}

* TestListener.java
```java
package net.limework.test;

import com.velocitypowered.api.event.Subscribe;

public class TestListener implements ITestListener<TestEvent> {
    @Override
    @Subscribe
    public void doSomething(TestEvent event) {
        System.out.println("test event fired " + event.x());
    }
}
  1. register TestListener.java

  2. fire the event TestEvent.java with example Code used to trigger the event

    AtomicInteger i = new AtomicInteger(0);
        server.getEventManager().register(this, new TestListener());
        server.getScheduler().buildTask(this, () -> {
            server.getEventManager().fireAndForget(new TestEvent(i.getAndIncrement()));
        }).repeat(Duration.ofSeconds(1)).schedule();
  3. see the console output

    [22:19:03 INFO]: test event fired 0
    [22:19:03 INFO]: test event fired 0
    [22:19:03 INFO]: Done (0.62s)!
    [22:19:04 INFO]: test event fired 1
    [22:19:04 INFO]: test event fired 1
    [22:19:05 INFO]: test event fired 2
    [22:19:05 INFO]: test event fired 2
    [22:19:06 INFO]: test event fired 3
    [22:19:06 INFO]: test event fired 3
    [22:19:07 INFO]: test event fired 4
    [22:19:07 INFO]: test event fired 4
    [22:19:08 INFO]: test event fired 5
    [22:19:08 INFO]: test event fired 5
novitpw commented 3 weeks ago

It's probably a good idea to stop using an interface whose methods need to be implemented in the listener class. This practice should not be used.

ham1255 commented 3 weeks ago

It's probably a good idea to stop using an interface whose methods need to be implemented in the listener class. This practice should not be used.

is there reason not to use this practice?

novitpw commented 3 weeks ago

It's probably a good idea to stop using an interface whose methods need to be implemented in the listener class. This practice should not be used.

is there reason not to use this practice?

This violates the design principle of event listeners. Velocity ​​doesn't have any listener interfaces like they do BungeeCord/Bukkit. In Velocity any object can be a listener. I advise you to simply register listeners on each platform individually. If you need some custom events, then make them separate for each platform and force users use specific APIs for each platform. This will be the easiest way.