Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ham1255 opened this issue Jun 6, 2024 · 3 comments

Comments

@ham1255
Copy link

ham1255 commented Jun 6, 2024

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
  • ITestEvent.java
package net.limework.test;

public interface ITestEvent {

    int x();
    
}
  • TestEvent.java
package net.limework.test;

public record TestEvent(int x) implements ITestEvent {

}
  • ITestListener.java
package net.limework.test;

public interface ITestListener<E extends ITestEvent> {

    void doSomething(E event);
    
}
  • TestListener.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();
  1. see the console output
[22:19:03 INFO]: test event fired 0
[22:19:03 INFO]: test event fired 0
[22:19:03 INFO]: Listening on /[0:0:0:0:0:0:0:0%0]:25577
[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
Copy link

novitpw commented Jun 12, 2024

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
Copy link
Author

ham1255 commented Jun 13, 2024

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
Copy link

novitpw commented Jun 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants