Skip to content

⚡ Offload Gem.setGems to async task #17

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.

Sign up for GitHub

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

Open
acsoto wants to merge 1 commit into
base: main
Choose a base branch
from
Open

⚡ Offload Gem.setGems to async task #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion modules/Gem/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@
<id>placeholderapi</id>
<url>https://repo.extendedclip.com/content/repositories/placeholderapi/</url>
</repository>
<repository>
<id>helpch</id>
<url>https://repo.helpch.at/releases/</url>
</repository>
</repositories>

<dependencies>
Expand All @@ -78,8 +82,32 @@
<dependency>
<groupId>me.clip</groupId>
<artifactId>placeholderapi</artifactId>
<version>2.11.1</version>
<version>2.11.5</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.13.2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>5.11.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>5.2.0</version>
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

There is a version mismatch between mockito-core (5.11.0) and mockito-inline (5.2.0). These should typically use the same version to avoid potential compatibility issues. Consider updating mockito-inline to 5.11.0 to match mockito-core, or verify that this specific version combination is intentional and compatible.

Suggested change
<version>5.2.0</version>
<version>5.11.0</version>

Copilot uses AI. Check for mistakes.
<scope>test</scope>
</dependency>
<dependency>
<groupId>net.bytebuddy</groupId>
<artifactId>byte-buddy</artifactId>
<version>1.14.17</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
15 changes: 11 additions & 4 deletions modules/Gem/src/main/java/com/mcatk/gem/command/CommandGem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public void run() {
}
switch (args[0].toLowerCase()) {
case "set":
set();
set(sender, args);
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The instance fields this.sender and this.args are still being set in onCommand() (lines 26-27), even though the set() method has been refactored to use parameters instead. This creates an incomplete fix: while set() is now thread-safe, the other methods (check(), add(), take(), total(), printHelp(), sendParameterError()) still rely on these instance fields. If multiple commands are executed concurrently or if async tasks complete after new commands arrive, these methods could read incorrect values. Consider either: (1) removing these instance fields entirely and passing parameters to all methods, or (2) making all database-accessing methods async with parameter passing like set() was changed.

Copilot uses AI. Check for mistakes.
break;
case "check":
check();
Expand All @@ -60,13 +60,20 @@ public void run() {
return true;
}

private void set() {
private void set(CommandSender sender, String[] args) {
if (args.length != 3) {
sendParameterError();
} else {
try {
Gem.getPlugin().getGemExecutor().setGems(args[1], Integer.parseInt(args[2]));
sender.sendMessage(Message.INFO + args[1] + " 的宝石设置为： " + args[2]);
final String playerName = args[1];
final int gems = Integer.parseInt(args[2]);
new BukkitRunnable() {
@Override
public void run() {
Gem.getPlugin().getGemExecutor().setGems(playerName, gems);
sender.sendMessage(Message.INFO + playerName + " 的宝石设置为： " + gems);
}
Comment on lines +70 to +75
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 1, 2026

Choose a reason for hiding this comment

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

P2 Badge Avoid calling Bukkit API from async thread

This async task now calls sender.sendMessage(...) from the scheduler’s async thread. In Bukkit/Spigot, most API calls (including messaging a CommandSender/Player) are not thread-safe and are expected to run on the main server thread. If sender is a player, this can surface as warnings or intermittent issues under load. The async work should be limited to the database call, then schedule a sync task to send the message back on the main thread.

Useful? React with 👍 / 👎.

}.runTaskAsynchronously(Gem.getPlugin());
} catch (NumberFormatException e) {
sender.sendMessage(Message.ERROR + "宝石必须是整数");
}
Expand Down
94 changes: 94 additions & 0 deletions modules/Gem/src/test/java/com/mcatk/gem/command/CommandGemBenchmarkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package com.mcatk.gem.command;

import com.mcatk.gem.Gem;
import com.mcatk.gem.GemExecutor;
import org.bukkit.Bukkit;
import org.bukkit.Server;
import org.bukkit.command.Command;
import org.bukkit.command.CommandSender;
import org.bukkit.plugin.Plugin;
import org.bukkit.scheduler.BukkitScheduler;
import org.bukkit.scheduler.BukkitTask;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.junit.MockitoJUnitRunner;

import java.lang.reflect.Field;
import java.util.logging.Logger;

import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.*;

@RunWith(MockitoJUnitRunner.class)
public class CommandGemBenchmarkTest {
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The class is named "CommandGemBenchmarkTest" but it's actually a functional test with a timing assertion rather than a true benchmark. Benchmarks typically involve multiple iterations, warmup phases, and statistical analysis. Consider renaming to "CommandGemAsyncTest" or similar to better reflect its purpose, or use a proper benchmarking framework like JMH if benchmarking is the actual goal.

Suggested change
public class CommandGemBenchmarkTest {
public class CommandGemAsyncTest {

Copilot uses AI. Check for mistakes.

private Gem gemMock;
private GemExecutor gemExecutorMock;
private CommandSender senderMock;
private Command commandMock;
private Server serverMock;
private BukkitScheduler schedulerMock;

@Before
public void setUp() throws Exception {
// Mock Gem.plugin (static field)
gemMock = mock(Gem.class);
Field pluginField = Gem.class.getDeclaredField("plugin");
pluginField.setAccessible(true);
pluginField.set(null, gemMock);

// Mock GemExecutor
gemExecutorMock = mock(GemExecutor.class);
lenient().when(gemMock.getGemExecutor()).thenReturn(gemExecutorMock);
lenient().when(gemMock.getLogger()).thenReturn(Logger.getLogger("GemTest"));

// Mock Bukkit.server (static)
serverMock = mock(Server.class);
Field serverField = Bukkit.class.getDeclaredField("server");
serverField.setAccessible(true);
serverField.set(null, serverMock);

// Mock Scheduler
schedulerMock = mock(BukkitScheduler.class);
lenient().when(serverMock.getScheduler()).thenReturn(schedulerMock);

// Mock generic runTaskAsynchronously
lenient().when(schedulerMock.runTaskAsynchronously(any(Plugin.class), any(Runnable.class)))
.thenReturn(mock(BukkitTask.class));

// Mock CommandSender
senderMock = mock(CommandSender.class);
when(senderMock.isOp()).thenReturn(true);

// Mock Command
commandMock = mock(Command.class);
}

@Test
public void testSetGemsAsync() {
CommandGem commandGem = new CommandGem();
String[] args = new String[]{"set", "player", "100"};

long startTime = System.currentTimeMillis();
commandGem.onCommand(senderMock, commandMock, "gem", args);
long endTime = System.currentTimeMillis();

long duration = endTime - startTime;
System.out.println("Execution time: " + duration + "ms");

// Assert that it returns immediately (non-blocking)
assertTrue("Execution should be async and take < 50ms, but took " + duration + "ms", duration < 50);

// Capture and run the task
ArgumentCaptor<Runnable> runnableCaptor = ArgumentCaptor.forClass(Runnable.class);
verify(schedulerMock).runTaskAsynchronously(any(Plugin.class), runnableCaptor.capture());

runnableCaptor.getValue().run();

// Verify logic executed
verify(gemExecutorMock).setGems("player", 100);
verify(senderMock).sendMessage(contains("100"));
}
}
Loading