⚡ Async Config Saving for ItemManager by acsoto · Pull Request #15 · mcac-cc/plugins

acsoto · 2026-02-01T16:23:45Z

Implemented asynchronous configuration saving for ItemManager to resolve performance issues caused by synchronous disk I/O when adding items.

Changes:

Added saveConfigAsync() method to ItemManager.
Replaced saveConfig() with saveConfigAsync() in ItemSort.addItem().
Introduced ExecutorService (ioExecutor) in ItemManager to handle file writes serially on a background thread.
Added ItemSortBenchmarkTest to verify the fix and ensure non-blocking execution.
Updated pom.xml to include testing dependencies (junit, mockito-inline, byte-buddy).

PR created automatically by Jules for task 18007997732134463950 started by @acsoto

Refactored ItemManager to use a dedicated single-threaded executor for configuration saving. Replaced synchronous `saveConfig()` calls in `ItemSort` with `saveConfigAsync()`. This prevents main thread blocking during item additions, eliminating redundant disk I/O. Added benchmark test `ItemSortBenchmarkTest` to verify performance improvement. Updated `pom.xml` with `junit`, `mockito-inline`, and `byte-buddy` dependencies for testing. Co-authored-by: acsoto <59144459+acsoto@users.noreply.github.com>

google-labs-jules · 2026-02-01T16:23:46Z

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8052467397

ℹ️ 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".

chatgpt-codex-connector · 2026-02-01T16:25:04Z

modules/ItemManager/src/main/java/com/mcatk/itemmanager/ItemManager.java

    public void onDisable() {
+        if (ioExecutor != null) {
+            ioExecutor.shutdown();
+        }


Flush async config writes before shutdown

Because saveConfigAsync() now queues writes on ioExecutor, calling shutdown() without waiting means any queued or in-progress config writes can be abandoned when the server stops or reloads. This is a regression from the previous synchronous saveConfig() behavior: if an item is added shortly before disable/reload, config.yml updates may never reach disk. Consider awaiting termination (or performing a final synchronous save) so that pending writes complete before shutdown.

Useful? React with 👍 / 👎.

Copilot

Pull request overview

Adds asynchronous config persistence to reduce main-thread blocking when adding items, and introduces a benchmark-style test plus new test dependencies.

Changes:

Added saveConfigAsync() with a single-threaded ExecutorService to serialize config file writes off the main thread.
Updated ItemSort.addItem() to call saveConfigAsync() instead of saveConfig().
Added a JUnit test and Maven test dependencies (JUnit/Mockito/Byte Buddy).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File	Description
modules/ItemManager/src/main/java/com/mcatk/itemmanager/ItemManager.java	Adds executor + async config write implementation and lifecycle handling.
modules/ItemManager/src/main/java/com/mcatk/itemmanager/ItemSort.java	Switches item add path to async config saving.
modules/ItemManager/src/test/java/com/mcatk/itemmanager/ItemSortBenchmarkTest.java	Adds benchmark-style regression test asserting async behavior.
modules/ItemManager/pom.xml	Adds test dependencies for the new unit test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-01T16:31:06Z

modules/ItemManager/pom.xml

+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-inline</artifactId>
+            <version>5.2.0</version>


mockito-inline 5.2.0 requires a newer Java runtime than this module targets (java.version is 1.8), so tests/build will fail under Java 8. Use a Mockito version compatible with Java 8 (e.g., Mockito 4.x or older) or raise the module’s Java target consistently across the build.

Suggested change

<version>5.2.0</version>

<version>4.11.0</version>

Copilot · 2026-02-01T16:31:07Z

modules/ItemManager/src/main/java/com/mcatk/itemmanager/ItemManager.java

+    public void saveConfigAsync() {
+        if (!isEnabled()) {
+            return;
+        }
+        if (getConfig() instanceof YamlConfiguration) {
+            final String data = ((YamlConfiguration) getConfig()).saveToString();
+            final File file = new File(getDataFolder(), "config.yml");
+            ioExecutor.submit(() -> {
+                try {


saveConfigAsync() submits work to ioExecutor without guarding against it being null or already shut down; submit() can throw RejectedExecutionException, which would currently propagate and potentially break callers. Add a defensive check/fallback (e.g., if executor is null/shutdown, call saveConfig() or log and return) and handle task submission failures gracefully.

Copilot · 2026-02-01T16:31:07Z

modules/ItemManager/src/main/java/com/mcatk/itemmanager/ItemManager.java

+            final String data = ((YamlConfiguration) getConfig()).saveToString();
+            final File file = new File(getDataFolder(), "config.yml");
+            ioExecutor.submit(() -> {
+                try {
+                    Files.write(file.toPath(), data.getBytes(StandardCharsets.UTF_8));
+                } catch (IOException ex) {
+                    getLogger().log(Level.SEVERE, "Could not save config to " + file, ex);
+                }
+            });


Files.write(...) to config.yml is not atomic; a crash/interruption mid-write can leave a partially written/empty config file. Prefer writing to a temporary file and then atomically moving/replacing the target (and ensure the data folder exists) to make async saves more robust.

Copilot · 2026-02-01T16:31:07Z

modules/ItemManager/src/test/java/com/mcatk/itemmanager/ItemSortBenchmarkTest.java

+        // Set static plugin field
+        Field pluginField = ItemManager.class.getDeclaredField("plugin");
+        pluginField.setAccessible(true);
+        pluginField.set(null, itemManager);
+


This test mutates ItemManager’s private static plugin field via reflection but never restores it. Add an @After teardown to reset the static back to null (or the prior value) to avoid cross-test pollution if additional tests are added/run in the same JVM.

Copilot · 2026-02-01T16:31:07Z

modules/ItemManager/src/test/java/com/mcatk/itemmanager/ItemSortBenchmarkTest.java

+        // Now it should be fast (< 100ms) because saveConfigAsync (mocked) does nothing
+        assertTrue("Should be fast (< 100ms) with async saving", duration < 100);
+
+        // Verify saveConfigAsync was called 50 times
+        verify(itemManager, times(iterations)).saveConfigAsync();
+
+        // Verify synchronous saveConfig was NOT called (except maybe in constructor? No, constructor calls config.getKeys)
+        // ItemSort constructor calls getConfig().getKeys(), not saveConfig().


The duration-based assertion (duration < 100) is likely to be flaky across environments/CI load because it depends on wall-clock timing and Mockito/mocking overhead. Prefer a deterministic assertion that the synchronous saveConfig() path is never invoked (which you already verify) and drop/relax the timing threshold (or make it a generous upper bound with rationale).

Suggested change

// Now it should be fast (< 100ms) because saveConfigAsync (mocked) does nothing

assertTrue("Should be fast (< 100ms) with async saving", duration < 100);

// Verify saveConfigAsync was called 50 times

verify(itemManager, times(iterations)).saveConfigAsync();

// Verify synchronous saveConfig was NOT called (except maybe in constructor? No, constructor calls config.getKeys)

// ItemSort constructor calls getConfig().getKeys(), not saveConfig().

// Verify saveConfigAsync was called 50 times

verify(itemManager, times(iterations)).saveConfigAsync();

// Verify synchronous saveConfig was NOT called (except maybe in constructor? No, constructor calls config.getKeys)

// ItemSort constructor calls getConfig().getKeys(), not saveConfig().

// Verify synchronous saveConfig was NOT called (except maybe in constructor? No, constructor calls config.getKeys)

// ItemSort constructor calls getConfig().getKeys(), not saveConfig().

Copilot · 2026-02-01T16:31:08Z

modules/ItemManager/pom.xml

+        <dependency>
+            <groupId>net.bytebuddy</groupId>
+            <artifactId>byte-buddy</artifactId>
+            <version>1.14.17</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>net.bytebuddy</groupId>
+            <artifactId>byte-buddy-agent</artifactId>
+            <version>1.14.17</version>
+            <scope>test</scope>
+        </dependency>


The explicit byte-buddy / byte-buddy-agent test dependencies are typically already pulled in (and version-aligned) by Mockito; pinning them separately can cause dependency/version conflicts. Prefer removing these direct dependencies unless there’s a specific need to override Mockito’s managed versions.

Suggested change

<dependency>

<groupId>net.bytebuddy</groupId>

<artifactId>byte-buddy</artifactId>

<version>1.14.17</version>

<scope>test</scope>

</dependency>

<dependency>

<groupId>net.bytebuddy</groupId>

<artifactId>byte-buddy-agent</artifactId>

<version>1.14.17</version>

<scope>test</scope>

</dependency>

Copilot · 2026-02-01T16:31:08Z

modules/ItemManager/src/main/java/com/mcatk/itemmanager/ItemManager.java

    public void onDisable() {
+        if (ioExecutor != null) {
+            ioExecutor.shutdown();
+        }


onDisable() only calls shutdown() and returns immediately. Any queued config writes may continue after the plugin is disabled (e.g., during /reload), which can race with a newly enabled plugin instance and overwrite newer config.yml contents. Consider awaiting termination (with a timeout) and/or cancelling queued tasks to ensure no background writes run after disable completes.

Copilot · 2026-02-01T16:31:08Z

modules/ItemManager/src/main/java/com/mcatk/itemmanager/ItemSort.java

        itemsHashMap.get(id1).getItemStackHashMap().put(id2, itemStack);
        ItemManager.getPlugin().getConfig().set(id1 + "." + id2, itemStack);
-        ItemManager.getPlugin().saveConfig();
+        ItemManager.getPlugin().saveConfigAsync();


This change triggers a full config serialization + queued disk write on every addItem call. Even though it’s off the main thread, high-frequency calls can build up a large executor queue and cause sustained disk churn. Consider coalescing/debouncing saves (e.g., schedule a single save after a short delay and collapse multiple calls) to keep throughput predictable.

Copilot · 2026-02-01T16:31:08Z

modules/ItemManager/src/test/java/com/mcatk/itemmanager/ItemSortBenchmarkTest.java

+            try {
+                Thread.sleep(10); // Simulate 10ms disk I/O
+            } catch (InterruptedException e) {
+                e.printStackTrace();


In the InterruptedException catch block, printing the stack trace loses the interrupt signal. Restore the interrupt flag (and avoid noisy stdout/stderr in tests) so the test runner can react appropriately.

Suggested change

e.printStackTrace();

Thread.currentThread().interrupt();

Copilot AI review requested due to automatic review settings February 1, 2026 16:23

chatgpt-codex-connector bot reviewed Feb 1, 2026

View reviewed changes

Copilot started reviewing on behalf of acsoto February 1, 2026 16:25 View session

Copilot AI reviewed Feb 1, 2026

View reviewed changes

Conversation

acsoto commented Feb 1, 2026

Uh oh!

google-labs-jules bot commented Feb 1, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant