Refactor Task execution to support asynchronous callbacks #3274
Conversation
PR SummaryThis PR refactors the task execution model to support asynchronous callbacks using Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 5d157d6: refactor: Refactor Task execution to support asynchronous callbacks
Files Processed (2)
- callback/src/main/java/com/iluwatar/callback/App.java (1 hunk)
- callback/src/main/java/com/iluwatar/callback/Task.java (1 hunk)
Actionable Comments (0)
Skipped Comments (1)
-
callback/src/main/java/com/iluwatar/callback/App.java [40-43]
enhancement: "Unnecessary sleep in main method."
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- b42dc28: fix: CallbackTest
Files Processed (1)
- callback/src/test/java/com/iluwatar/callback/CallbackTest.java (2 hunks)
Actionable Comments (1)
-
callback/src/test/java/com/iluwatar/callback/CallbackTest.java [57-57]
possible bug: "Test flakiness due to hardcoded timeout."
Skipped Comments (0)
|
|
||
| task.executeWith(callback); | ||
|
|
||
| latch.await(5, TimeUnit.SECONDS); |
There was a problem hiding this comment.
The test uses await(5, TimeUnit.SECONDS) which might lead to flaky tests if the task takes longer than 5 seconds to complete. Consider using a more robust mechanism for waiting, such as a timeout with a more appropriate duration or a different synchronization primitive.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 12a7f1d: chore: code style
Files Processed (2)
- callback/src/main/java/com/iluwatar/callback/Task.java (1 hunk)
- callback/src/test/java/com/iluwatar/callback/CallbackTest.java (2 hunks)
Actionable Comments (1)
-
callback/src/test/java/com/iluwatar/callback/CallbackTest.java [58-70]
possible bug: "Improve test robustness by using a more reliable waiting mechanism."
Skipped Comments (0)
| latch.await(5, TimeUnit.SECONDS); | ||
|
|
||
| assertEquals(Integer.valueOf(1), callingCount, "Callback called once"); | ||
|
|
||
| callingCount = 0; | ||
| latch = new CountDownLatch(1); | ||
|
|
||
| task.executeWith(callback); | ||
|
|
||
| assertEquals(Integer.valueOf(2), callingCount, "Callback called twice"); | ||
| latch.await(5, TimeUnit.SECONDS); | ||
|
|
||
| assertEquals(Integer.valueOf(1), callingCount, "Callback called once again"); | ||
| } |
There was a problem hiding this comment.
The test uses await(5, TimeUnit.SECONDS), which might lead to flaky tests if the task takes longer than 5 seconds. Consider using a more robust waiting mechanism, such as a timeout with a more appropriate duration or a different synchronization primitive.
| CompletableFuture.runAsync( | ||
| () -> { | ||
| execute(); | ||
| if (callback != null) { | ||
| callback.call(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
In general, looks good! However, I'd like to keep the synchronous callbacks (previous functionality) as well.
Could we demonstrate both sychronous and asynchronous callback in the example? The guidance would be something like
- If your operation is short and straightforward, keep it synchronous.
- If the operation can block or is resource-intensive, prefer asynchronous callbacks to maintain application responsiveness.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (2)
Files Processed (4)
- callback/src/main/java/com/iluwatar/callback/App.java (1 hunk)
- callback/src/main/java/com/iluwatar/callback/Callback.java (1 hunk)
- callback/src/main/java/com/iluwatar/callback/Task.java (1 hunk)
- callback/src/test/java/com/iluwatar/callback/CallbackTest.java (2 hunks)
Actionable Comments (0)
Skipped Comments (1)
-
callback/src/test/java/com/iluwatar/callback/CallbackTest.java [61-84]
possible issue: "Timeout in asynchronous test might mask issues."
|
|
Hi @iluwatar , thanks for your feedback! I’ve just pushed updates to:
Please let me know if there’s anything else you’d like me to adjust! Thanks again for reviewing. |
Summary
This PR refactors the existing Task execution model to support asynchronous callbacks. The
executeWithmethod has been modified to execute tasks asynchronously usingCompletableFuture, allowing the main thread to remain non-blocking. Upon task completion, the provided callback is invoked.Changes
CompletableFuture.runAsync().Motivation
The original implementation was synchronous, potentially blocking the main thread. By introducing asynchronous execution, we improve the responsiveness and scalability of the application, especially in scenarios involving long-running tasks.In addition, Microsoft's Event-based Asynchronous Pattern (EAP) also emphasizes the use of callbacks to handle notifications and result processing after asynchronous operations.
Testing
Please review the changes and provide feedback.