GH-1052 Fix ignore calling synchronous events in async code #1052
Conversation
…epo (we do not do anything with it). Remove CompletableFuture from service. Make sure events run sync.
WalkthroughThe changes update how ignore and un-ignore actions work in the codebase. Methods that previously worked asynchronously and returned a result now just perform their tasks without waiting for completion or returning any value. The commands that use these methods have been updated to match this new approach, and a scheduler is now used to run some of these operations. Notifications to users are now sent right after the action is triggered, without waiting for confirmation that the action finished. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
📦 Development Build ReadyWarning Do not use this build in production. It is for testing purposes only and may be unstable. |
|
Alternative fixup proposed by @vLuckyyy - changes are done in only one service. package com.eternalcode.core.feature.ignore;
import com.eternalcode.commons.scheduler.Scheduler;
import com.eternalcode.core.event.EventCaller;
import com.eternalcode.core.feature.ignore.event.IgnoreAllEvent;
import com.eternalcode.core.feature.ignore.event.IgnoreEvent;
import com.eternalcode.core.feature.ignore.event.UnIgnoreAllEvent;
import com.eternalcode.core.feature.ignore.event.UnIgnoreEvent;
import com.eternalcode.core.injector.annotations.Inject;
import com.eternalcode.core.injector.annotations.component.Service;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;
@Service
class IgnoreServiceImpl implements IgnoreService {
private final IgnoreRepository ignoreRepository;
private final EventCaller caller;
private final Scheduler scheduler;
@Inject
IgnoreServiceImpl(IgnoreRepository ignoreRepository, EventCaller caller, Scheduler scheduler) {
this.ignoreRepository = ignoreRepository;
this.caller = caller;
this.scheduler = scheduler;
}
@Override
public CompletableFuture<Boolean> isIgnored(UUID requester, UUID target) {
return this.ignoreRepository.isIgnored(requester, target);
}
@Override
public CompletableFuture<Boolean> ignore(UUID requester, UUID target) {
CompletableFuture<Boolean> eventResult = this.scheduler.complete(() -> {
IgnoreEvent event = this.caller.callEvent(new IgnoreEvent(requester, target));
return event.isCancelled();
});
return eventResult.thenCompose(cancelled -> {
if (cancelled) {
return CompletableFuture.completedFuture(true);
}
return this.ignoreRepository.ignore(requester, target)
.thenApply(unused -> false);
});
}
@Override
public CompletableFuture<Boolean> ignoreAll(UUID requester) {
CompletableFuture<Boolean> eventResult = this.scheduler.complete(() -> {
IgnoreAllEvent event = this.caller.callEvent(new IgnoreAllEvent(requester));
return event.isCancelled();
});
return eventResult.thenCompose(cancelled -> {
if (cancelled) {
return CompletableFuture.completedFuture(true);
}
return this.ignoreRepository.ignoreAll(requester)
.thenApply(unused -> false);
});
}
@Override
public CompletableFuture<Boolean> unIgnore(UUID requester, UUID target) {
CompletableFuture<Boolean> eventResult = this.scheduler.complete(() -> {
UnIgnoreEvent event = this.caller.callEvent(new UnIgnoreEvent(requester, target));
return event.isCancelled();
});
return eventResult.thenCompose(cancelled -> {
if (cancelled) {
return CompletableFuture.completedFuture(true);
}
return this.ignoreRepository.unIgnore(requester, target)
.thenApply(unused -> false);
});
}
@Override
public CompletableFuture<Boolean> unIgnoreAll(UUID requester) {
CompletableFuture<Boolean> eventResult = this.scheduler.complete(() -> {
UnIgnoreAllEvent event = this.caller.callEvent(new UnIgnoreAllEvent(requester));
return event.isCancelled();
});
return eventResult.thenCompose(cancelled -> {
if (cancelled) {
return CompletableFuture.completedFuture(true);
}
return this.ignoreRepository.unIgnoreAll(requester)
.thenApply(unused -> false);
});
}
} |
CitralFlo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/IgnoreCommand.java (1)
54-63: Notification timing could be problematicThe notification is sent inside the scheduled task. If the ignore operation fails, users will still get a success message. Consider moving the notification outside or adding error handling.
♻️ Duplicate comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/UnIgnoreCommand.java (1)
54-63: Same notification timing issue as IgnoreCommandLike in IgnoreCommand, the success notification is sent inside the scheduled task without checking if the operation succeeded.
eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/IgnoreServiceImpl.java (2)
61-61: Remove the pointless thenApplySame issue - the
.thenApply(unused -> true)serves no purpose here.- this.ignoreRepository.unIgnore(requester, target).thenApply(unused -> true); + this.ignoreRepository.unIgnore(requester, target);
72-72: Another unnecessary thenApplyRemove this one too.
- this.ignoreRepository.unIgnoreAll(requester).thenApply(unused -> true); + this.ignoreRepository.unIgnoreAll(requester);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
eternalcore-api/src/main/java/com/eternalcode/core/feature/ignore/IgnoreService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/IgnoreCommand.java(4 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/IgnoreServiceImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/UnIgnoreCommand.java(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/IgnoreCommand.java (1)
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.
eternalcore-api/src/main/java/com/eternalcode/core/feature/ignore/IgnoreService.java (2)
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.
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.
eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/IgnoreServiceImpl.java (2)
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.
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.
🔇 Additional comments (1)
eternalcore-api/src/main/java/com/eternalcode/core/feature/ignore/IgnoreService.java (1)
10-16: Consider the implications of void returnsChanging to
voidmeans callers can't know if operations succeeded or handle errors. This could lead to incorrect user notifications (success messages even when operations fail).
| this.ignoreService.ignoreAll(senderUuid); | ||
|
|
||
| this.noticeService.create() | ||
| .player(senderUuid) | ||
| .notice(translation -> translation.privateChat().ignoreAll()) | ||
| .send(); |
There was a problem hiding this comment.
🛠️ 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.
| 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.
eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/UnIgnoreCommand.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/ignore/IgnoreServiceImpl.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| return this.ignoreRepository.ignoreAll(requester).thenApply(unused -> true); | ||
| this.ignoreRepository.ignoreAll(requester).thenApply(unused -> true); |
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
Added error handling from commons in this space
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Caution
CodeRabbit plans did not produce any file changes.
| } | ||
|
|
||
| return this.ignoreRepository.ignoreAll(requester).thenApply(unused -> true); | ||
| this.ignoreRepository.ignoreAll(requester).thenApply(unused -> FutureHandler.whenSuccess(null)); |
Remove complicated logic from commands, drop waiting for success of repo (we do not do anything with it). Remove CompletableFuture from service. Make sure events run sync.