Skip to content

GH-1052 Fix ignore calling synchronous events in async code #1052

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.

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ public interface IgnoreService {

CompletableFuture<Boolean> isIgnored(UUID requester, UUID target);

CompletableFuture<Boolean> ignore(UUID requester, UUID target);
void ignore(UUID requester, UUID target);

CompletableFuture<Boolean> ignoreAll(UUID requester);
void ignoreAll(UUID requester);

CompletableFuture<Boolean> unIgnore(UUID requester, UUID target);
void unIgnore(UUID requester, UUID target);

CompletableFuture<Boolean> unIgnoreAll(UUID requester);
void unIgnoreAll(UUID requester);

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.eternalcode.core.feature.ignore;

import com.eternalcode.annotations.scan.command.DescriptionDocs;
import com.eternalcode.commons.scheduler.Scheduler;
import com.eternalcode.core.injector.annotations.Inject;
import com.eternalcode.core.notice.NoticeService;
import com.eternalcode.core.user.User;
Expand All @@ -19,11 +20,13 @@ class IgnoreCommand {

private final IgnoreService ignoreService;
private final NoticeService noticeService;
private final Scheduler scheduler;

@Inject
IgnoreCommand(IgnoreService ignoreService, NoticeService noticeService) {
IgnoreCommand(IgnoreService ignoreService, NoticeService noticeService, Scheduler scheduler) {
this.ignoreService = ignoreService;
this.noticeService = noticeService;
this.scheduler = scheduler;
}

@Execute
Expand All @@ -48,10 +51,9 @@ void ignore(@Context User sender, @Arg User target) {
return;
}

this.ignoreService.ignore(senderUuid, targetUuid).thenAccept(cancelled -> {
if (cancelled) {
return;
}

this.scheduler.run(() -> {
this.ignoreService.ignore(senderUuid, targetUuid);

this.noticeService.create()
.player(senderUuid)
Expand All @@ -62,21 +64,19 @@ void ignore(@Context User sender, @Arg User target) {
});
}


@Execute(name = "-all", aliases = "*")
@DescriptionDocs(description = "Ignore all players")
void ignoreAll(@Context User sender) {
UUID senderUuid = sender.getUniqueId();

this.ignoreService.ignoreAll(senderUuid).thenAccept(cancelled -> {
if (cancelled) {
return;
}
this.ignoreService.ignoreAll(senderUuid);

this.noticeService.create()
.player(senderUuid)
.notice(translation -> translation.privateChat().ignoreAll())
.send();
Comment on lines +73 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Inconsistent execution pattern

The ignoreAll method calls the service directly while ignore uses the scheduler. This creates an inconsistent pattern - both should use the same approach for predictability.

-        this.ignoreService.ignoreAll(senderUuid);
-
-        this.noticeService.create()
-            .player(senderUuid)
-            .notice(translation -> translation.privateChat().ignoreAll())
-            .send();
+        this.scheduler.run(() -> {
+            this.ignoreService.ignoreAll(senderUuid);
+
+            this.noticeService.create()
+                .player(senderUuid)
+                .notice(translation -> translation.privateChat().ignoreAll())
+                .send();
+        });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
this.ignoreService.ignoreAll(senderUuid);
this.noticeService.create()
.player(senderUuid)
.notice(translation -> translation.privateChat().ignoreAll())
.send();
this.scheduler.run(() -> {
this.ignoreService.ignoreAll(senderUuid);
this.noticeService.create()
.player(senderUuid)
.notice(translation -> translation.privateChat().ignoreAll())
.send();
});
🤖 Prompt for AI Agents
In
eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/IgnoreCommand.java
around lines 73 to 78, the ignoreAll method calls the ignoreService directly
while the ignore method uses a scheduler, causing inconsistency. To fix this,
modify the ignoreAll call to also use the scheduler for execution, matching the
pattern used by ignore, ensuring both methods execute asynchronously and
consistently.


this.noticeService.create()
.player(senderUuid)
.notice(translation -> translation.privateChat().ignoreAll())
.send();
});
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -29,45 +29,46 @@ public CompletableFuture<Boolean> isIgnored(UUID requester, UUID target) {
}

@Override
public CompletableFuture<Boolean> ignore(UUID requester, UUID target) {
public void ignore(UUID requester, UUID target) {
IgnoreEvent event = this.caller.callEvent(new IgnoreEvent(requester, target));

if (event.isCancelled()) {
return CompletableFuture.completedFuture(false);
return;
}

return this.ignoreRepository.ignore(requester, target).thenApply(unused -> true);
this.ignoreRepository.ignore(requester, target);
}

@Override
public CompletableFuture<Boolean> ignoreAll(UUID requester) {
public void ignoreAll(UUID requester) {
IgnoreAllEvent event = this.caller.callEvent(new IgnoreAllEvent(requester));

if (event.isCancelled()) {
return CompletableFuture.completedFuture(false);
return;
}

return this.ignoreRepository.ignoreAll(requester).thenApply(unused -> true);
this.ignoreRepository.ignoreAll(requester).thenApply(unused -> true);
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 16, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Unnecessary operation that does nothing

The .thenApply(unused -> true) creates a new CompletableFuture that's immediately thrown away. Just call the repository method without chaining.

-        this.ignoreRepository.ignoreAll(requester).thenApply(unused -> true);
+        this.ignoreRepository.ignoreAll(requester);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
this.ignoreRepository.ignoreAll(requester).thenApply(unused -> true);
this.ignoreRepository.ignoreAll(requester);
🤖 Prompt for AI Agents
In
eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/IgnoreServiceImpl.java
at line 50, remove the unnecessary .thenApply(unused -> true) chaining from the
call to ignoreRepository.ignoreAll(requester) since it creates a new
CompletableFuture that is not used. Simply call
ignoreRepository.ignoreAll(requester) directly without chaining to avoid
redundant operations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added error handling from commons in this space

Copy link
Contributor

Choose a reason for hiding this comment

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

@CitralFlo Thanks for the clarification! I understand you've added error handling from commons in this space.

I notice that this line uses .thenApply(unused -> true) while the other methods in the same file use FutureHandler.whenSuccess(null). Should this be consistent with the other methods for proper error handling?

// Other methods use:
this.ignoreRepository.ignore(requester, target).thenApply(unused -> FutureHandler.whenSuccess(null));

// But ignoreAll currently uses:
this.ignoreRepository.ignoreAll(requester).thenApply(unused -> true);

Would you like me to update my suggestion to align with the error handling pattern used in the other methods?

🐰
( o.o )
> ^ <


🧠 Learnings used
Learnt from: CitralFlo
PR: EternalCodeTeam/EternalCore#896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleServiceImpl.java:24-27
Timestamp: 2025-01-28T21:35:47.750Z
Learning: In EternalCore, async operations using CompletableFuture should remain non-blocking. Avoid using .join() or similar blocking operations to maintain better performance and scalability.

Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalCore#865
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveService.java:0-0
Timestamp: 2024-10-14T20:11:13.610Z
Learning: In the 'EternalCore' project, avoid using null in method signatures in Java code.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes please

Copy link
Contributor

Choose a reason for hiding this comment

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

Caution

CodeRabbit plans did not produce any file changes.

}

@Override
public CompletableFuture<Boolean> unIgnore(UUID requester, UUID target) {
public void unIgnore(UUID requester, UUID target) {
UnIgnoreEvent event = this.caller.callEvent(new UnIgnoreEvent(requester, target));

if (event.isCancelled()) {
return CompletableFuture.completedFuture(false);
return;
}
return this.ignoreRepository.unIgnore(requester, target).thenApply(unused -> true);

this.ignoreRepository.unIgnore(requester, target).thenApply(unused -> true);
}

@Override
public CompletableFuture<Boolean> unIgnoreAll(UUID requester) {
public void unIgnoreAll(UUID requester) {
UnIgnoreAllEvent event = this.caller.callEvent(new UnIgnoreAllEvent(requester));

if (event.isCancelled()) {
return CompletableFuture.completedFuture(false);
return;
}
return this.ignoreRepository.unIgnoreAll(requester).thenApply(unused -> true);
}

this.ignoreRepository.unIgnoreAll(requester).thenApply(unused -> true);
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.eternalcode.core.feature.ignore;

import com.eternalcode.annotations.scan.command.DescriptionDocs;
import com.eternalcode.commons.scheduler.Scheduler;
import com.eternalcode.core.injector.annotations.Inject;
import com.eternalcode.core.notice.NoticeService;
import com.eternalcode.core.user.User;
Expand All @@ -19,11 +20,13 @@ class UnIgnoreCommand {

private final IgnoreService ignoreService;
private final NoticeService noticeService;
private final Scheduler scheduler;

@Inject
public UnIgnoreCommand(IgnoreService ignoreService, NoticeService noticeService) {
public UnIgnoreCommand(IgnoreService ignoreService, NoticeService noticeService, Scheduler scheduler) {
this.ignoreService = ignoreService;
this.noticeService = noticeService;
this.scheduler = scheduler;
}

@Execute
Expand All @@ -48,10 +51,9 @@ void ignore(@Context User sender, @Arg User target) {
return;
}

this.ignoreService.unIgnore(senderUuid, targetUuid).thenAccept(cancelled -> {
if (cancelled) {
return;
}

this.scheduler.run(() -> {
this.ignoreService.unIgnore(senderUuid, targetUuid);

this.noticeService.create()
.player(senderUuid)
Expand All @@ -67,16 +69,12 @@ void ignore(@Context User sender, @Arg User target) {
void unIgnoreAll(@Context User sender) {
UUID senderUuid = sender.getUniqueId();

this.ignoreService.unIgnoreAll(senderUuid).thenAccept(cancelled -> {
if (cancelled) {
return;
}
this.ignoreService.unIgnoreAll(senderUuid);

this.noticeService.create()
.player(senderUuid)
.notice(translation -> translation.privateChat().unIgnoreAll())
.send();
});
}
this.noticeService.create()
.player(senderUuid)
.notice(translation -> translation.privateChat().unIgnoreAll())
.send();

}
}