Skip to content

Conversation

@vLuckyyy
Copy link
Member

@vLuckyyy vLuckyyy commented Feb 9, 2026

No description provided.

@vLuckyyy vLuckyyy requested a review from a team as a code owner February 9, 2026 21:21
@vLuckyyy vLuckyyy changed the title Add tpa event api GH-1298 Add tpa event api Feb 9, 2026
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

Comment on lines +47 to +54
.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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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()))

Comment on lines 58 to +68
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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));

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

Successfully merging this pull request may close these issues.

1 participant