Fix Owlplug scanner hangs and timeouts #317
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CommandRunner
participant ExecutorService
participant StreamReader
participant Process
Caller->>CommandRunner: run(command...)
CommandRunner->>Process: Start process
CommandRunner->>ExecutorService: submit(StreamReader)
ExecutorService->>StreamReader: call()
StreamReader->>Process: Read process output
alt Timeout enabled
CommandRunner->>Process: waitFor(timeout)
alt Process finishes in time
CommandRunner->>ExecutorService: get output (Future)
else Timeout exceeded
CommandRunner->>Process: destroyForcibly
CommandRunner->>Caller: throw IOException
end
else No timeout
CommandRunner->>Process: waitFor()
CommandRunner->>ExecutorService: get output (Future)
end
CommandRunner->>ExecutorService: shutdown
CommandRunner->>Caller: return CommandResult
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 (
|
|
@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: 0
🧹 Nitpick comments (2)
owlplug-host/src/main/java/com/owlplug/host/io/CommandRunner.java (2)
85-96: Proper output retrieval with resource cleanup.The implementation correctly:
- Retrieves output with a timeout for graceful completion
- Handles exceptions during output retrieval
- Ensures executor shutdown in a finally block
One suggestion:
- // Let 1 seconds for gracefully read and complete process + // Allow 1 second for graceful reading and process completion
115-133: Well-designed StreamReader implementation.The StreamReader class is a good encapsulation of the input stream reading logic.
Consider adding resource cleanup to prevent potential resource leaks:
@Override public String call() throws Exception { - BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream)); - StringBuilder output = new StringBuilder(); - String line; - while ((line = reader.readLine()) != null) { - output.append(line).append(System.lineSeparator()); - } - return output.toString(); + try (BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream))) { + StringBuilder output = new StringBuilder(); + String line; + while ((line = reader.readLine()) != null) { + output.append(line).append(System.lineSeparator()); + } + return output.toString(); + } }This ensures the reader is properly closed even in case of exceptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
owlplug-host/src/main/java/com/owlplug/host/io/CommandRunner.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
owlplug-host/src/main/java/com/owlplug/host/io/CommandRunner.java (4)
23-31: Appropriate imports added for concurrency implementation.The additional imports are necessary for the new asynchronous process handling approach and are correctly defined.
59-63: Good use of ExecutorService for asynchronous output reading.Using a single-threaded executor to handle potentially blocking I/O operations separately from the main thread is a good approach. This helps prevent the main thread from hanging while reading process output.
64-83: Improved timeout handling with better error management.The implementation properly uses Java's built-in process API features:
- Uses
process.waitFor(timeout, TimeUnit.MILLISECONDS)when timeout is activated- Properly tracks completion with a boolean flag
- Adds explicit error logging and resource cleanup when interrupted
- Forcibly destroys the process on timeout with clear error messaging
This should effectively address the scanner hangs and timeouts mentioned in the PR.
55-96: Overall refactoring significantly improves process handling and reliability.The refactoring of the
runmethod effectively addresses the scanner hangs and timeouts mentioned in the PR by:
- Moving from synchronous to asynchronous output reading
- Using Java's built-in timeout capabilities instead of manual time tracking
- Improving error handling and resource cleanup
- Properly handling process termination on timeout
This implementation should be more robust and less prone to hanging or timing out unexpectedly.
DropSnorz
force-pushed
the
fix/scanner-hang
branch
from
95fe9e6 to
ec7e08f
Compare
Related to #316