Conversation
Refactored MedalShowCmd to schedule broadcastMessage and sendMessage calls on the main thread using Bukkit.getScheduler().runTask(). Removed shared state (sender, args) from MedalShowCmd to fix race conditions during concurrent command execution. Added JUnit 5 and Mockito dependencies and a unit test to verify the fix and prevent regressions. Modified SQLManager to allow dependency injection for testing. Co-authored-by: acsoto <59144459+acsoto@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3b531f50f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Bukkit.getScheduler().runTaskAsynchronously(MedalCabinet.getPlugin(), () -> { | ||
| StringBuilder stringBuilder = new StringBuilder(prefix).append("§e").append(sender.getName()) | ||
| .append(" 展示了他的勋章：\n"); | ||
| for (Medal medal : SQLManager.getInstance().getPlayerMedals(sender.getName())) { | ||
| stringBuilder.append(medal).append(" "); |
There was a problem hiding this comment.
The async task still dereferences the CommandSender to call sender.getName() (and again in the showMe async block). Accessing Bukkit API objects off the main thread is unsafe, which means this change still risks the same thread-safety violations the refactor intended to remove. This can surface when the async task runs while the player object is being mutated or has already disconnected. Capture the player name (or UUID) on the main thread before scheduling the async work, and only use that captured value inside the async task.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request fixes unsafe asynchronous API usage in the MedalShowCmd command executor by ensuring that Bukkit API calls are executed on the main thread. The changes eliminate race conditions by removing shared class-level fields and passing parameters through method arguments instead.
Changes:
- Refactored
MedalShowCmdto wrap Bukkit API calls (broadcastMessageandsendMessage) inrunTaskto ensure main-thread execution - Removed class-level
senderandargsfields fromMedalShowCmdto eliminate race conditions - Added test infrastructure support in
SQLManagerand comprehensive testing dependencies
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| MedalShowCmd.java | Removed shared fields and wrapped Bukkit API calls in runTask for thread safety |
| SQLManager.java | Added setInstance method and changed constructor visibility for test support |
| MedalShowCmdTest.java | Added unit test to verify async tasks properly schedule sync operations |
| pom.xml | Added JUnit Jupiter, Mockito, and Byte Buddy dependencies for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Test | ||
| public void testShowAll() { | ||
| MedalShowCmd cmd = new MedalShowCmd(); | ||
| String[] args = {"all"}; | ||
|
|
||
| // Mock SQL return | ||
| ArrayList<Medal> medals = new ArrayList<>(); | ||
| medals.add(new Medal("1", "TestMedal", "STONE", "Desc")); | ||
| when(mockedSQLManager.getPlayerMedals("TestPlayer")).thenReturn(medals); | ||
|
|
||
| // Capture the async task | ||
| ArgumentCaptor<Runnable> asyncTaskCaptor = ArgumentCaptor.forClass(Runnable.class); | ||
| when(mockedScheduler.runTaskAsynchronously(eq(mockedPlugin), asyncTaskCaptor.capture())) | ||
| .thenReturn(mock(BukkitTask.class)); | ||
|
|
||
| // Execute command | ||
| cmd.onCommand(mockedPlayer, mock(Command.class), "medalshow", args); | ||
|
|
||
| // Run the captured async task | ||
| Runnable asyncTask = asyncTaskCaptor.getValue(); | ||
| asyncTask.run(); | ||
|
|
||
| // Verify that runTask (sync) was called | ||
| ArgumentCaptor<Runnable> syncTaskCaptor = ArgumentCaptor.forClass(Runnable.class); | ||
| verify(mockedScheduler).runTask(eq(mockedPlugin), syncTaskCaptor.capture()); | ||
|
|
||
| // Run the captured sync task | ||
| Runnable syncTask = syncTaskCaptor.getValue(); | ||
| syncTask.run(); | ||
|
|
||
| // Verify broadcastMessage is called | ||
| verify(mockedServer).broadcastMessage(contains("TestPlayer 展示了他的勋章")); | ||
| } |
There was a problem hiding this comment.
The test only covers the "all" command case (testShowAll) but does not test the "me" command case which uses sender.sendMessage() instead of broadcastMessage(). Both code paths should be tested to ensure comprehensive coverage of the async-to-sync scheduling fix. Consider adding a testShowMe() method that verifies the "me" command also properly schedules the sendMessage call on the main thread.
| <repository> | ||
| <id>helpch</id> | ||
| <url>https://repo.helpch.at/releases/</url> | ||
| </repository> |
There was a problem hiding this comment.
The helpch repository was added but none of the dependencies in the pom.xml appear to require it. All test dependencies (JUnit Jupiter, Mockito, Byte Buddy) are available from Maven Central (sonatype repository). Unless there's a specific reason for this repository, it should be removed to avoid unnecessary repository lookups during builds.
| <repository> | |
| <id>helpch</id> | |
| <url>https://repo.helpch.at/releases/</url> | |
| </repository> |
| if ("all".equals(args[0])) { | ||
| showAll(); | ||
| showAll(sender); | ||
| } else if ("me".equals(args[0])) { | ||
| showMe(); | ||
| showMe(sender); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
The onCommand method returns false when args[0] is "all" or "me", which in Bukkit's CommandExecutor contract indicates the command was not handled successfully and will cause the usage message to be sent to the player. This should return true to indicate successful command execution. The return value should be true for all valid command cases.
| import org.bukkit.plugin.Plugin; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.ArgumentCaptor; | ||
| import org.mockito.MockedStatic; | ||
|
|
||
| import java.lang.reflect.Field; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; |
There was a problem hiding this comment.
Several imports appear to be unused in the test: Plugin (line 12), Collections (line 21). These should be removed to keep the code clean and avoid confusion.
|
|
||
| private SQLManager() { | ||
| // For testing | ||
| public static void setInstance(SQLManager instance) { |
There was a problem hiding this comment.
The setInstance method allows replacing the singleton instance, which could lead to unexpected behavior in production if accidentally called. Consider adding appropriate safeguards such as making this package-private or adding a check to ensure it's only called during testing (e.g., checking for test environment). Alternatively, consider using a proper dependency injection framework or test-specific subclasses.
| public static void setInstance(SQLManager instance) { | |
| static void setInstance(SQLManager instance) { |
| <version>1.14.19</version> | ||
| <scope>test</scope> |
There was a problem hiding this comment.
The PR description mentions that this is compatible with Java 21, but the pom.xml still specifies Java 1.8 (line 15). This creates a discrepancy between the claimed Java 21 support and the actual project configuration. If the dependencies are truly meant to be compatible with Java 21, consider updating the java.version property to 21 or at least to a more recent version that can leverage the features of these newer dependencies.
| syncTask.run(); | ||
|
|
||
| // Verify broadcastMessage is called | ||
| verify(mockedServer).broadcastMessage(contains("TestPlayer 展示了他的勋章")); |
There was a problem hiding this comment.
The test verifies that broadcastMessage is called on the server (mockedServer.broadcastMessage), but the actual implementation calls MedalCabinet.getPlugin().getServer().broadcastMessage(). While both should work since mockedPlugin.getServer() returns mockedServer, it would be more accurate to also verify the call on Bukkit.broadcastMessage() or document why the server's broadcastMessage is verified instead, as Bukkit.broadcastMessage() is a static convenience method that delegates to the server.
| if ("all".equals(args[0])) { | ||
| showAll(); | ||
| showAll(sender); | ||
| } else if ("me".equals(args[0])) { | ||
| showMe(); | ||
| showMe(sender); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
When args[0] is provided but doesn't match "all" or "me", the command silently does nothing and returns false. Consider adding an else clause to show the help message or an error message for invalid arguments, similar to how printHelp is shown when args.length == 0. This would provide better user experience and feedback.
| import org.mockito.ArgumentCaptor; | ||
| import org.mockito.MockedStatic; | ||
|
|
||
| import java.lang.reflect.Field; |
There was a problem hiding this comment.
The Field import (line 19) is unused and should be removed to keep the imports clean.
| import java.lang.reflect.Field; |
| // Setup SQLManager singleton | ||
| mockedSQLManagerStatic.when(SQLManager::getInstance).thenReturn(mockedSQLManager); |
There was a problem hiding this comment.
The test uses mockStatic for SQLManager but never actually uses the setInstance method that was added in this PR. The test correctly mocks SQLManager.getInstance() to return the mocked instance, but the setInstance method (lines 19-21 in SQLManager.java) is not utilized in the test. This suggests the setInstance method may not be necessary for testing purposes, or the test could be simplified by using it directly instead of mocking the static getInstance call.
1 participant
💡 What:
MedalShowCmd.javato ensureBukkit.broadcastMessageandCommandSender.sendMessageare called on the main thread usingrunTask.senderandargsinMedalShowCmdto eliminate race conditions.SQLManager.javato support dependency injection for testing viasetInstanceand protected constructor.MedalShowCmdTest.javato verify the fix and ensure safe scheduling.pom.xmlto includejunit-jupiter,mockito-core,mockito-inline, andbyte-buddy(compatible with Java 21) for testing.🎯 Why:
senderin theCommandExecutor, which would cause data corruption if multiple players executed the command simultaneously.📊 Measured Improvement:
MedalShowCmdTestpasses, confirming that the message sending logic is now correctly scheduled to run on the main thread.PR created automatically by Jules for task 2142753520230428981 started by @acsoto