-
-
Notifications
You must be signed in to change notification settings - Fork 21
GH-1298 Add tpa event api #1298
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new API for teleportation request events (PreTeleportRequestEvent and TeleportRequestEvent) and integrates them into the existing tpa and tpahere commands. The changes are logical and the new event API is well-defined.
However, I've found a few issues:
- There are duplicate registrations of a command and a listener in the example plugin, which could lead to bugs.
- More critically, there are several instances where a synchronous Bukkit event is being called from an asynchronous thread. This is a significant threading issue that can cause server instability and must be addressed.
My review includes detailed comments on these points with suggestions for how to resolve them.
| .send(); | ||
|
|
||
| this.requestService.createRequest(player.getUniqueId(), target.getUniqueId()); | ||
| this.eventCaller.callEvent(new TeleportRequestEvent(player, target)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are calling a synchronous Bukkit event (TeleportRequestEvent) from an asynchronous context (CompletableFuture#thenAccept). This is not thread-safe and can lead to severe concurrency issues. Bukkit API calls, including firing events, must be performed on the main server thread unless the event is explicitly asynchronous.
To resolve this, you need to schedule the event call on the main thread. For example, using Bukkit's scheduler:
Bukkit.getScheduler().runTask(pluginInstance, () -> this.eventCaller.callEvent(new TeleportRequestEvent(player, target)));You will need to get access to your plugin instance to do this. A better approach might be to inject a scheduler service that abstracts this away, which seems to be a pattern in this project.
|
|
||
| this.requestService.createRequest(sender.getUniqueId(), target.getUniqueId()); | ||
| this.requestService.createRequest(sender.getUniqueId(), target.getUniqueId()); | ||
| this.eventCaller.callEvent(new TeleportRequestEvent(sender, target)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are calling a synchronous Bukkit event (TeleportRequestEvent) from an asynchronous context (CompletableFuture#thenAccept). This is not thread-safe and can lead to severe concurrency issues. Bukkit API calls, including firing events, must be performed on the main server thread unless the event is explicitly asynchronous.
To resolve this, you need to schedule the event call on the main thread. For example, using Bukkit's scheduler:
Bukkit.getScheduler().runTask(pluginInstance, () -> this.eventCaller.callEvent(new TeleportRequestEvent(sender, target)));You will need to get access to your plugin instance to do this. A better approach might be to inject a scheduler service that abstracts this away, which seems to be a pattern in this project.
|
|
||
| this.requestService.createRequest(sender.getUniqueId(), target.getUniqueId()); | ||
|
|
||
| this.eventCaller.callEvent(new TeleportRequestEvent(sender, target)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same threading issue as in the execute method is present here. You are calling a synchronous Bukkit event (TeleportRequestEvent) from an asynchronous context (CompletableFuture#thenAccept) inside a loop. This is not thread-safe and can lead to severe concurrency issues. Bukkit API calls, including firing events, must be performed on the main server thread.
To resolve this, you need to schedule the event call on the main thread. For example, using Bukkit's scheduler:
Bukkit.getScheduler().runTask(pluginInstance, () -> this.eventCaller.callEvent(new TeleportRequestEvent(sender, target)));You will need to get access to your plugin instance to do this. A better approach might be to inject a scheduler service that abstracts this away.
| .commands( | ||
| new ApiAfkCommand(provide.getAfkService()), | ||
| new ApiIgnoreCommand(provide.getIgnoreService()), | ||
| new ApiJailCommand(provide.getJailService()), | ||
| new ApiRandomTeleportCommand(provide.getRandomTeleportService()), | ||
| new ApiSpawnCommand(provide.getSpawnService()), | ||
| new ApiRandomTeleportCommand(provide.getRandomTeleportService()), | ||
| new ApiHomeCommand(provide.getHomeService())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ApiRandomTeleportCommand is registered twice. This will likely cause an error during command registration as the same command will be registered multiple times. Please remove the duplicate entry on line 53.
.commands(
new ApiAfkCommand(provide.getAfkService()),
new ApiIgnoreCommand(provide.getIgnoreService()),
new ApiJailCommand(provide.getJailService()),
new ApiRandomTeleportCommand(provide.getRandomTeleportService()),
new ApiSpawnCommand(provide.getSpawnService()),
new ApiHomeCommand(provide.getHomeService()))| Stream.of( | ||
| new ApiAfkListener(), | ||
| new CatBoyListener(provide.getCatboyService()), | ||
| new ApiRandomTeleportListener(provide.getRandomTeleportService()), | ||
| new ApiPrivateChatListener(server), | ||
| new ApiRandomTeleportListener(provide.getRandomTeleportService()), | ||
| new ApiHomeListener(server), | ||
| new ApiJailListener(server), | ||
| new ApiIgnoreListener() | ||
| ).forEach(listener -> server.getPluginManager().registerEvents(listener, this)); | ||
| new ApiAfkListener(), | ||
| new CatBoyListener(provide.getCatboyService()), | ||
| new ApiRandomTeleportListener(provide.getRandomTeleportService()), | ||
| new ApiPrivateChatListener(server), | ||
| new ApiRandomTeleportListener(provide.getRandomTeleportService()), | ||
| new ApiHomeListener(server), | ||
| new ApiJailListener(server), | ||
| new ApiIgnoreListener(), | ||
| new ApiTeleportRequestListener()) | ||
| .forEach(listener -> server.getPluginManager().registerEvents(listener, this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ApiRandomTeleportListener is registered twice. This will cause its event handlers to be called twice for each event, which is likely not intended and can lead to bugs. Please remove the duplicate entry on line 63.
Stream.of(
new ApiAfkListener(),
new CatBoyListener(provide.getCatboyService()),
new ApiRandomTeleportListener(provide.getRandomTeleportService()),
new ApiPrivateChatListener(server),
new ApiHomeListener(server),
new ApiJailListener(server),
new ApiIgnoreListener(),
new ApiTeleportRequestListener())
.forEach(listener -> server.getPluginManager().registerEvents(listener, this));
No description provided.