Refactor task runner to use CompletableFuture and remove deprecated ListenableFuture #399
Conversation
WalkthroughTaskRunner moved from ListenableFuture to CompletableFuture with an executor-backed submission path and WorkerStateEvent handlers; Task/TaskResult APIs were simplified (success() → completed(), TaskResult now always reports completed), and multiple tasks adjusted to return completed(). Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as TaskRunner.scheduleNext()
participant Submit as submitCompletable()
participant Exec as Executor
participant Worker as Task Worker
participant Event as WorkerStateEvent
participant Future as CompletableFuture
participant UI as UI (TaskBarController)
rect rgb(240,248,255)
Note over Scheduler,Submit: New submission flow (high-level)
Scheduler->>Submit: submit task
Submit->>Future: create CompletableFuture
Submit->>Exec: execute task
Exec->>Worker: run
par Success
Worker->>Event: SUCCEEDED
Event->>Future: complete()
Future->>UI: thenAccept -> UI update (Platform.runLater)
and Failure
Worker->>Event: FAILED
Event->>Future: completeExceptionally()
Future->>UI: exceptionally -> log & UI report
and Cancel
Worker->>Event: CANCELLED
Event->>Future: cancel()
end
Future->>Scheduler: continuation (remove current, schedule next)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java (3)
27-28: LGTM! Clean migration to CompletableFuture with improved field immutability.The import changes correctly reflect the migration from deprecated ListenableFuture to CompletableFuture. Making the executor, taskQueue, and taskHistory fields final is good practice and ensures they cannot be reassigned after construction.
Also applies to: 32-32, 53-55, 57-57
60-62: LGTM! Modern initialization with diamond operators.The constructor initialization is clean and uses appropriate diamond operators.
84-84: LGTM! More descriptive logging.The updated log message "Task submitted to queue head" better reflects the behavior of submitTaskOnQueueHead.
owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java
Show resolved
Hide resolved
owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java
Show resolved
Hide resolved
…cated ListenableFuture
DropSnorz
force-pushed
the
chore/completable-future
branch
from
8a8459e to
482d8d7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java (1)
144-155: Consider removing event handlers after firing to reduce memory footprint.While the developer noted that the CompletableFuture will be garbage collected, the JavaFX Task objects are retained in
taskHistory(up to 10 tasks) with their event handlers still attached. Each handler holds a reference to its corresponding CompletableFuture, creating a retention chain.For cleaner resource management, consider storing handler references and removing them after the CompletableFuture completes:
private CompletableFuture<TaskResult> submitCompletable(AbstractTask task, Executor exec) { CompletableFuture<TaskResult> cf = new CompletableFuture<>(); - task.addEventHandler(WorkerStateEvent.WORKER_STATE_SUCCEEDED, - e -> cf.complete(task.getValue())); - task.addEventHandler(WorkerStateEvent.WORKER_STATE_FAILED, - e -> cf.completeExceptionally(task.getException())); - task.addEventHandler(WorkerStateEvent.WORKER_STATE_CANCELLED, - e -> cf.cancel(true)); + + EventHandler<WorkerStateEvent> successHandler = e -> { + cf.complete(task.getValue()); + task.removeEventHandler(WorkerStateEvent.WORKER_STATE_SUCCEEDED, successHandler); + task.removeEventHandler(WorkerStateEvent.WORKER_STATE_FAILED, failHandler); + task.removeEventHandler(WorkerStateEvent.WORKER_STATE_CANCELLED, cancelHandler); + }; + EventHandler<WorkerStateEvent> failHandler = e -> { + cf.completeExceptionally(task.getException()); + task.removeEventHandler(WorkerStateEvent.WORKER_STATE_SUCCEEDED, successHandler); + task.removeEventHandler(WorkerStateEvent.WORKER_STATE_FAILED, failHandler); + task.removeEventHandler(WorkerStateEvent.WORKER_STATE_CANCELLED, cancelHandler); + }; + EventHandler<WorkerStateEvent> cancelHandler = e -> { + cf.cancel(true); + task.removeEventHandler(WorkerStateEvent.WORKER_STATE_SUCCEEDED, successHandler); + task.removeEventHandler(WorkerStateEvent.WORKER_STATE_FAILED, failHandler); + task.removeEventHandler(WorkerStateEvent.WORKER_STATE_CANCELLED, cancelHandler); + }; + + task.addEventHandler(WorkerStateEvent.WORKER_STATE_SUCCEEDED, successHandler); + task.addEventHandler(WorkerStateEvent.WORKER_STATE_FAILED, failHandler); + task.addEventHandler(WorkerStateEvent.WORKER_STATE_CANCELLED, cancelHandler); exec.execute(task); return cf; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java(4 hunks)owlplug-client/src/main/java/com/owlplug/core/tasks/AbstractTask.java(1 hunks)owlplug-client/src/main/java/com/owlplug/core/tasks/TaskResult.java(1 hunks)owlplug-client/src/main/java/com/owlplug/explore/tasks/BundleInstallTask.java(1 hunks)owlplug-client/src/main/java/com/owlplug/explore/tasks/SourceSyncTask.java(1 hunks)owlplug-client/src/main/java/com/owlplug/plugin/tasks/DirectoryRemoveTask.java(1 hunks)owlplug-client/src/main/java/com/owlplug/plugin/tasks/FileSyncTask.java(1 hunks)owlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginRemoveTask.java(1 hunks)owlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginScanTask.java(1 hunks)owlplug-client/src/main/java/com/owlplug/plugin/tasks/SymlinkRemoveTask.java(1 hunks)owlplug-client/src/main/java/com/owlplug/project/tasks/PluginLookupTask.java(1 hunks)owlplug-client/src/main/java/com/owlplug/project/tasks/ProjectSyncTask.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T18:35:06.941Z
Learnt from: DropSnorz
Repo: DropSnorz/OwlPlug PR: 359
File: owlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginScanTask.java:104-113
Timestamp: 2025-11-03T18:35:06.941Z
Learning: In OwlPlug's PluginScanTask (owlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginScanTask.java), all plugin paths are absolute paths, never relative, as they are normalized via FileUtils.convertPath before being stored or used as directory scopes.
Applied to files:
owlplug-client/src/main/java/com/owlplug/project/tasks/PluginLookupTask.javaowlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginScanTask.javaowlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginRemoveTask.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (14)
owlplug-client/src/main/java/com/owlplug/plugin/tasks/DirectoryRemoveTask.java (1)
37-51: Consistent TaskResult completion usageReturning
completed()after a successful delete keeps the previous control flow (exceptions still bubble) while aligning with the new unified completion semantics and ensuring a non‑nullTaskResulton success.owlplug-client/src/main/java/com/owlplug/plugin/tasks/FileSyncTask.java (1)
55-79: Return ofcompleted()matches existing error modelOn normal completion you now return
completed(), while failures still throwTaskException. This keeps semantics intact and matches the updatedTaskResult/AbstractTaskAPI.owlplug-client/src/main/java/com/owlplug/explore/tasks/SourceSyncTask.java (1)
130-140: Completed result correctly reflects existing “best‑effort” semanticsYou now return
completed()after finishing sync and messaging, while network/parse issues still surface only as logged warnings. That preserves the previous behavior and fits the new TaskResult contract.owlplug-client/src/main/java/com/owlplug/plugin/tasks/SymlinkRemoveTask.java (1)
36-54: Unified completion signaling for symlink removalThe new
return completed();on the success path cleanly harmonizes this task with the rest of the framework while leaving failure behavior (exception on delete error) unchanged.owlplug-client/src/main/java/com/owlplug/project/tasks/PluginLookupTask.java (1)
41-65: TaskResult completion aligned with lookup flowReturning
completed()after the lookup loop and final progress update is consistent with the revised TaskResult API and keeps the task’s observable behavior the same.owlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginScanTask.java (1)
85-95: Clear mapping between success and exceptional completionSwitching from the old success signaling to
return completed();in the try block, while still wrapping failures inTaskExceptionin the catch, cleanly supports the new CompletableFuture/TaskResult model without changing scan behavior.owlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginRemoveTask.java (1)
47-76: Non‑null completion result on successful plugin removalThe new
completed()return on the success branch ensures callers always receive aTaskResultfor successful removals, while failures still throwTaskException. This matches the project‑wide completion refactor.owlplug-client/src/main/java/com/owlplug/project/tasks/ProjectSyncTask.java (1)
49-95: Project sync now returns a concrete completed TaskResultAfter synchronizing projects and updating progress/messages, returning
completed()makes this task consistent with the new TaskResult/TaskRunner contract while keeping the functional behavior identical.owlplug-client/src/main/java/com/owlplug/explore/tasks/BundleInstallTask.java (1)
127-127: LGTM! Method rename aligns with the AbstractTask refactoring.The change from
success()tocompleted()is consistent with the project-wide migration to CompletableFuture-based task completion signaling.owlplug-client/src/main/java/com/owlplug/core/tasks/AbstractTask.java (1)
89-91: LGTM! Method rename improves semantic clarity.Renaming
success()tocompleted()better aligns with CompletableFuture terminology and accurately represents task completion signaling across the codebase.owlplug-client/src/main/java/com/owlplug/core/tasks/TaskResult.java (1)
23-25: LGTM! Simplified semantics align with CompletableFuture architecture.The change to have
isCompleted()always returntrueis correct in the new CompletableFuture-based design. When a task throws an exception, the CompletableFuture completes exceptionally and TaskResult is never examined. TaskResult instances only exist for successfully completed tasks, so the constanttruereturn value is semantically accurate.owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java (3)
52-61: LGTM! Field finalization and inline initialization improve immutability.Making the executor, task queue, and task history fields
finaland initializing them directly in the constructor is a good defensive coding practice that prevents accidental reassignment.
83-83: LGTM! Log message clarification improves debugging.The updated log message "Task submitted to queue head" more clearly indicates priority submission versus regular queue submission.
103-130: LGTM! CompletableFuture flow correctly handles success and error paths with proper thread synchronization.The refactored scheduling logic properly:
- Uses
thenAcceptfor successful completion- Uses
exceptionallyfor error handling with appropriate null checks (lines 113-124)- Wraps JavaFX UI updates in
Platform.runLaterto ensure thread safety- Maintains task lifecycle (remove and schedule next) in both paths
No description provided.