Conversation
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: 5cf5964bfc
ℹ️ 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".
| private final ExecutorService ioExecutor = Executors.newSingleThreadExecutor(new ThreadFactory() { | ||
| @Override | ||
| public Thread newThread(Runnable r) { | ||
| Thread t = new Thread(r, "GemShop-IO-Thread"); | ||
| t.setDaemon(true); | ||
| return t; |
There was a problem hiding this comment.
Because the executor thread is marked as a daemon and never shut down, any pending async config writes can be dropped on server shutdown or plugin reload, and stale queued writes from an old plugin instance can still run and overwrite newer config data. This is most visible if an item is added/removed and the server/plugin is stopped or reloaded shortly after. Consider shutting down the executor and awaiting completion (or switching to Bukkit’s async task scheduler with proper shutdown hooks) so saves are guaranteed to finish.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR optimizes configuration saving in the ItemShop by implementing asynchronous I/O to prevent blocking the main thread during item addition/deletion operations. The optimization moves disk writes to a background thread while ensuring thread safety by serializing configuration data on the main thread.
Changes:
- Implemented async config saving using a single-threaded ExecutorService in ItemShop.java to offload blocking I/O operations
- Added benchmark tests to verify the performance improvement and async behavior
- Fixed dependency issues in POMs (Gem artifactId case correction, PlaceholderAPI repository and version updates)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| modules/GemShop/src/main/java/com/mcatk/gemshop/shops/itemshop/ItemShop.java | Added ExecutorService and async config saving logic to replace blocking saveConfig() calls |
| modules/GemShop/src/test/java/com/mcatk/gemshop/shops/itemshop/ItemShopBenchmarkTest.java | Added benchmark tests to verify async behavior and performance improvements |
| modules/GemShop/pom.xml | Fixed Gem dependency artifactId and added test dependencies (junit, mockito-inline, byte-buddy) |
| modules/Gem/pom.xml | Added helpch repository and updated PlaceholderAPI version to fix dependency resolution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private final ExecutorService ioExecutor = Executors.newSingleThreadExecutor(new ThreadFactory() { | ||
| @Override | ||
| public Thread newThread(Runnable r) { | ||
| Thread t = new Thread(r, "GemShop-IO-Thread"); | ||
| t.setDaemon(true); | ||
| return t; | ||
| } | ||
| }); |
There was a problem hiding this comment.
The ExecutorService is never shut down, which will prevent the JVM from exiting cleanly and may cause resource leaks. Since ItemShop is instantiated once during plugin initialization (in ShopFactory constructor), you need to add a shutdown method to properly clean up the executor when the plugin is disabled. Consider adding a public method like shutdown() that calls ioExecutor.shutdown() or ioExecutor.shutdownNow(), and invoke this from GemShop's onDisable() method.
| @After | ||
| public void tearDown() throws Exception { | ||
| // Reset the static field | ||
| Field pluginField = GemShop.class.getDeclaredField("plugin"); | ||
| pluginField.setAccessible(true); | ||
| pluginField.set(null, null); | ||
| } |
There was a problem hiding this comment.
The test creates an ItemShop instance with an ExecutorService but never shuts it down. This can lead to test thread leaks and prevent the test JVM from exiting cleanly. Consider adding a cleanup step in the tearDown method to shut down the executor, or create a shutdown method in ItemShop that can be called during test cleanup.
| if (duration >= 50) { | ||
| throw new RuntimeException("Execution time too slow: " + duration + "ms. Optimization failed?"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Using a RuntimeException for test failures is not the standard JUnit pattern. The test should use JUnit assertions like assertTrue(duration < 50, "Execution time too slow...") or assertThat(duration).isLessThan(50). This would provide better test reporting and integration with test runners.
| if (duration >= 50) { | |
| throw new RuntimeException("Execution time too slow: " + duration + "ms. Optimization failed?"); | |
| } | |
| } | |
| assertTrue("Execution time too slow: " + duration + "ms. Optimization failed?", duration < 50); | |
| } | |
| } |
| import java.lang.reflect.Field; | ||
| import java.util.logging.Logger; | ||
|
|
||
| import static org.junit.Assert.assertTrue; |
There was a problem hiding this comment.
The import for assertTrue is unused. It should be removed or the test should use proper JUnit assertions instead of throwing RuntimeException.
| import static org.junit.Assert.assertTrue; |
| @Override | ||
| public Thread newThread(Runnable r) { | ||
| Thread t = new Thread(r, "GemShop-IO-Thread"); | ||
| t.setDaemon(true); |
There was a problem hiding this comment.
While daemon threads prevent JVM hang on shutdown, they also mean that pending writes can be abruptly terminated when the plugin disables, potentially leading to data loss or corrupted config files. Consider making the thread non-daemon, or implementing a graceful shutdown mechanism that waits for pending writes to complete using awaitTermination() with a reasonable timeout in a shutdown method.
| private void saveConfigAsync() { | ||
| if (!(GemShop.getPlugin().getConfig() instanceof YamlConfiguration)) { | ||
| GemShop.getPlugin().saveConfig(); | ||
| return; | ||
| } | ||
|
|
||
| YamlConfiguration yaml = (YamlConfiguration) GemShop.getPlugin().getConfig(); | ||
| final String data = yaml.saveToString(); | ||
| final File file = new File(GemShop.getPlugin().getDataFolder(), "config.yml"); |
There was a problem hiding this comment.
If GemShop.getPlugin() returns null (which could happen during certain initialization or shutdown sequences), this will throw a NullPointerException. Consider adding a null check at the start of the method to handle this edge case gracefully.
| public void run() { | ||
| try { | ||
| if (file.getParentFile() != null && !file.getParentFile().exists()) { | ||
| file.getParentFile().mkdirs(); |
There was a problem hiding this comment.
The mkdirs() method returns a boolean indicating success or failure, but this return value is ignored. If directory creation fails, Files.write() will subsequently throw an IOException, which is caught but only logged. Consider checking the return value of mkdirs() and logging a more specific error message if directory creation fails, to help with debugging configuration issues.
| file.getParentFile().mkdirs(); | |
| boolean created = file.getParentFile().mkdirs(); | |
| if (!created && !file.getParentFile().exists()) { | |
| GemShop.getPlugin().getLogger().severe("Could not create parent directories for config.yml, aborting async save."); | |
| return; | |
| } |
| } | ||
|
|
||
| public void delItem(String shopId, String itemId) { | ||
| itemsMap.get(shopId).getMap().remove(itemId); |
There was a problem hiding this comment.
The delItem method does not check if itemsMap.get(shopId) is null before calling getMap().remove(). If called with an invalid shopId, this will throw a NullPointerException. Consider adding a null check and appropriate error handling or logging to prevent crashes from invalid input.
| itemsMap.get(shopId).getMap().remove(itemId); | |
| Items items = itemsMap.get(shopId); | |
| if (items == null) { | |
| GemShop.getPlugin().getLogger().warning( | |
| "Attempted to delete item '" + itemId + "' from unknown shopId '" + shopId + "'."); | |
| return; | |
| } | |
| items.getMap().remove(itemId); |
| @Test | ||
| public void testAddItemPerformance() { | ||
| // Setup slow saveConfig (which should NOT be called now) | ||
| doAnswer(new Answer<Void>() { | ||
| @Override | ||
| public Void answer(InvocationOnMock invocation) throws Throwable { | ||
| Thread.sleep(50); // Simulate 50ms Disk I/O | ||
| return null; | ||
| } | ||
| }).when(mockPlugin).saveConfig(); | ||
|
|
||
| ItemShop itemShop = new ItemShop(); | ||
|
|
||
| long startTime = System.currentTimeMillis(); | ||
| // Price "100" as string | ||
| itemShop.addItem(mockPlayer, "shop1", "item1", "100"); | ||
| long endTime = System.currentTimeMillis(); | ||
|
|
||
| long duration = endTime - startTime; | ||
| System.out.println("Benchmark Execution time: " + duration + "ms"); | ||
|
|
||
| // Assert that saveConfig was NOT called (because we use async save) | ||
| verify(mockPlugin, never()).saveConfig(); | ||
|
|
||
| // Assert that saveToString WAS called | ||
| verify(mockConfig).saveToString(); | ||
|
|
||
| // In optimized code, duration should be near 0ms | ||
| if (duration >= 50) { | ||
| throw new RuntimeException("Execution time too slow: " + duration + "ms. Optimization failed?"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test measures execution time immediately after calling addItem, but the actual file write happens asynchronously. This test doesn't verify that the async write actually completes successfully - it only verifies that the main thread doesn't block. Consider adding a mechanism to wait for the async operation to complete (e.g., using CountDownLatch or similar) and verify that the file write was actually attempted.
| // Fix NPE for new File(dataFolder, ...) | ||
| File tempFolder = new File(System.getProperty("java.io.tmpdir")); | ||
| when(mockPlugin.getDataFolder()).thenReturn(tempFolder); |
There was a problem hiding this comment.
The test writes to the system temp directory but doesn't clean up the config.yml file afterwards. This can leave test artifacts on the filesystem and potentially cause test interactions. Consider creating a unique temporary directory for each test run and cleaning it up in tearDown, or mock the file writing completely to avoid actual I/O during unit tests.
1 participant
ItemShop.javausing a single-threadedExecutorService.saveConfig()was blocking the main thread during item addition/deletion, causing performance hiccups.saveToString()on the main thread to ensure thread-safe access to Bukkit's configuration API.Gem/pom.xmlto fix build issues (PlaceholderAPI dependency resolution).junit,mockito-inline,byte-buddy) toGemShop/pom.xml.PR created automatically by Jules for task 13388166261087698001 started by @acsoto