LineWriter, NonBlockingLineWriter & various fixes & clean-up#18
Conversation
…kingLineWriter Task
Summary of ChangesHello @vorburger, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the task execution framework by introducing new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces LineWriter interfaces and NonBlockingLineWriter implementation for asynchronous logging, along with various fixes and improvements to the task execution framework. It includes migration from Thread.sleep to a safer Threads utility and general code cleanup.
- Adds LineWriter interface and NonBlockingLineWriter for non-blocking output handling
- Migrates from Thread.sleep to Threads.sleep utility for proper interrupt handling
- Improves task execution framework with better timeout handling and logging
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dev/enola/common/log/JulConfigurer.java | New utility for configuring Java logging with custom formatter |
| src/dev/enola/common/function/CheckedConsumer.java | New functional interface for consumers that can throw checked exceptions |
| src/dev/enola/common/concurrent/Threads.java | Moved from demo package to common with visibility change |
| src/dev/enola/be/task/test/SlowTask.java | Updated to use Threads.sleep and Duration instead of raw milliseconds |
| src/dev/enola/be/task/demo/LongIncrementingTask.java | Enhanced with NonBlockingLineWriter integration and performance improvements |
| src/dev/enola/be/task/TaskWithoutInputOutput.java | New abstract base class for tasks without input/output |
| src/dev/enola/be/task/TaskExecutorServices.java | Removed unused close method |
| src/dev/enola/be/task/TaskExecutor.java | Improved timeout handling and task lifecycle management |
| src/dev/enola/be/task/TaskCallable.java | Removed duplicate endedAt call |
| src/dev/enola/be/task/Task.java | Enhanced documentation and improved toString output formatting |
| src/dev/enola/be/io/NonBlockingLineWriter.java | New non-blocking line writer implementation with overflow handling |
| src/dev/enola/be/io/LineWriters.java | Utility class with common LineWriter implementations |
| src/dev/enola/be/io/LineWriter.java | New interface for line-based output operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| JulConfigurer.configureRootLogger(); | ||
|
|
||
| // Count to max, with sleep pause between each increment | ||
| var max = 1000000L; |
There was a problem hiding this comment.
[nitpick] The hardcoded value 1000000L should be extracted to a named constant to improve code readability and maintainability.
| while (!Thread.currentThread().isInterrupted()) { | ||
| if (overflow) { | ||
| // https://en.wikipedia.org/wiki/Ellipsis | ||
| delegate.println("[… output ... ※ ... truncated …]"); |
There was a problem hiding this comment.
[nitpick] The overflow message should be extracted to a named constant to avoid duplication and improve maintainability.
| var timeout = task.timeout(); | ||
| if (!timeout.isZero() && !timeout.isNegative()) { | ||
| timeoutScheduler.schedule( | ||
| () -> futureTask.cancel(true), timeout.toMillis(), MILLISECONDS); |
There was a problem hiding this comment.
The timeout cancellation runs asynchronously but there's no handling for potential race conditions if the task completes just as the timeout fires. Consider checking task completion status before cancelling.
| () -> futureTask.cancel(true), timeout.toMillis(), MILLISECONDS); | |
| () -> { | |
| if (!futureTask.isDone()) { | |
| futureTask.cancel(true); | |
| } | |
| }, timeout.toMillis(), MILLISECONDS); |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable LineWriter abstraction with a non-blocking implementation, which is excellent for handling task output without blocking. The refactoring of TaskExecutor to improve timeout handling and shutdown logic significantly enhances correctness and robustness. The addition of common utilities like Threads, CheckedConsumer, and JulConfigurer is also a good move. I've identified a critical concurrency bug and a high-severity usability issue in the new NonBlockingLineWriter that should be addressed.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| var input = new Input(max, sleep); | ||
| var task = new LongIncrementingTask(input, System.out::println); | ||
| var pumperTask = new NonBlockingLineWriter(13, LineWriters.SYSTEM_OUT); |
There was a problem hiding this comment.
The magic number 13 for queue capacity should be extracted to a named constant or variable to explain its significance and make it configurable.
| while (!Thread.currentThread().isInterrupted()) { | ||
| if (overflow) { | ||
| // https://en.wikipedia.org/wiki/Ellipsis | ||
| delegate.println("[… output ... ※ ... truncated …]"); |
There was a problem hiding this comment.
The overflow message contains a mix of different ellipsis characters ('…' and '...'). Consider using consistent ellipsis formatting throughout the message.
| delegate.println("[… output ... ※ ... truncated …]"); | |
| delegate.println("[… output … ※ … truncated …]"); |
No description provided.