Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.