fix(tool): prevent pipe buffer deadlock in ShellCommandTool#619
Conversation
Summary of ChangesHello @fang-tech, 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 provides a crucial fix for a pipe buffer deadlock within the 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
|
fang-tech
force-pushed
the
fix/tool/ShellCommandTool-issue-617
branch
from
d504fe8 to
7220687
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a critical deadlock issue in ShellCommandTool by implementing asynchronous reading of process output streams. The use of CompletableFuture is a good approach. The accompanying tests are thorough and cover various scenarios that could trigger the deadlock. My review includes suggestions to further improve the robustness and maintainability of the implementation, such as using a dedicated executor for I/O tasks to prevent resource contention, replacing magic numbers with constants, and improving the clarity and correctness of the new tests.
I am having trouble creating individual review comments. Click here to see my feedback.
agentscope-core/src/main/java/io/agentscope/core/tool/coding/ShellCommandTool.java (363-366)
The CompletableFuture.supplyAsync calls are using the default common ForkJoinPool. This pool is primarily designed for CPU-bound tasks. Since reading from a process stream is a blocking I/O operation, using the common pool can lead to thread starvation if many shell commands are executed concurrently, potentially impacting the performance of the entire application.
It's recommended to use a dedicated Executor for these I/O-bound tasks. Since the class already uses Schedulers.boundedElastic() for blocking operations, you can leverage its underlying executor.
stdoutFuture =
CompletableFuture.supplyAsync(() -> readStream(finalProcess.getInputStream()), Schedulers.boundedElastic().asExecutor());
stderrFuture =
CompletableFuture.supplyAsync(() -> readStream(finalProcess.getErrorStream()), Schedulers.boundedElastic().asExecutor());
agentscope-core/src/main/java/io/agentscope/core/tool/coding/ShellCommandTool.java (387-388)
The timeout value 1 is a magic number. It would be better to define this as a named constant at the top of the class, for example private static final long PARTIAL_OUTPUT_WAIT_SECONDS = 1;. This improves readability and makes the value's purpose clear.
agentscope-core/src/main/java/io/agentscope/core/tool/coding/ShellCommandTool.java (411-412)
The timeout value 5 is a magic number. It would be better to define this as a named constant, for example private static final long FULL_OUTPUT_WAIT_SECONDS = 5;. This improves readability and makes it easier to configure if needed.
agentscope-core/src/test/java/io/agentscope/core/tool/coding/ShellCommandToolTest.java (785-934)
The comments and assertion messages in this test class repeatedly refer to a fix using 'Apache Commons Exec's PumpStreamHandler'. This is inaccurate, as the actual implementation uses CompletableFuture for asynchronous stream reading. These references are misleading and should be updated to reflect the current implementation to avoid confusion. For example, '...with Apache Commons Exec fix...' should be changed to '...with the async stream reading fix...'.
agentscope-core/src/test/java/io/agentscope/core/tool/coding/ShellCommandToolTest.java (827)
The method signature uses throws Exception, which is a broad declaration. It's better practice to declare the specific checked exceptions that the method might throw, such as java.io.IOException. This makes the code clearer about what kind of errors to expect and handle.
void reproduceLargeFileCatDeadlock() throws java.io.IOException {
agentscope-core/src/test/java/io/agentscope/core/tool/coding/ShellCommandToolTest.java (876-877)
Using URL.getPath() can be brittle and may fail on some systems or build environments if the path to the class files contains spaces or other special characters that get URL-encoded. A more robust way to convert a resource URL to a file path is to use URL.toURI(). Note that this will require adding throws Exception to the method signature.
String resourcePath =
java.nio.file.Paths.get(getClass().getClassLoader().getResource("large_output_test.txt").toURI()).toString();
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical deadlock issue in ShellCommandTool that occurs when executing commands producing large outputs (>4-64KB) that exceed pipe buffer capacity. The fix implements asynchronous stream reading using CompletableFuture to continuously drain stdout and stderr pipes, preventing the child process from blocking while waiting for buffer space.
Changes:
- Implemented asynchronous stream reading with CompletableFuture to prevent pipe buffer deadlock
- Added getOutputWithTimeout() helper method for safe async stream result retrieval
- Added comprehensive test coverage with 4 test cases covering various large output scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/tool/coding/ShellCommandTool.java | Added async stream reading using CompletableFuture.supplyAsync and helper method getOutputWithTimeout to prevent deadlock when child processes produce large output |
| agentscope-core/src/test/java/io/agentscope/core/tool/coding/ShellCommandToolTest.java | Added BufferDeadlockTests nested class with 4 test cases covering seq command, temporary file cat, test resource file cat, and yes|head command scenarios |
| agentscope-core/src/test/resources/large_output_test.txt | Added 1000-line test resource file (~100KB) to simulate large output for deadlock testing |
Comments suppressed due to low confidence (1)
agentscope-core/src/test/java/io/agentscope/core/tool/coding/ShellCommandToolTest.java:933
- The test comments reference "Apache Commons Exec fix" and "PumpStreamHandler" (lines 807-808, 815, 891, 920, 923), but the actual implementation uses CompletableFuture.supplyAsync with custom async stream readers, not Apache Commons Exec library. These misleading comments could confuse future maintainers about the actual implementation approach. The comments should be updated to accurately describe the CompletableFuture-based asynchronous stream reading solution.
@Test
@DisplayName("Should handle large output without deadlock using seq command")
@EnabledOnOs({OS.LINUX, OS.MAC})
void reproducePipeBufferDeadlockWithSeq() {
// Generate ~8KB output using seq command, which exceeds typical pipe buffer
// (4-8KB)
// This command completes in milliseconds with the fix (Apache Commons Exec's
// PumpStreamHandler)
String command = "seq 1 20000"; // Approximately 8000 bytes
// Set a reasonable timeout - should complete quickly now
Mono<ToolResultBlock> result = tool.executeShellCommand(command, 60);
StepVerifier.create(result)
.assertNext(
block -> {
String text = extractText(block);
// Expected: Should complete successfully without timeout
// The fix uses PumpStreamHandler to consume output in separate
// threads
assertFalse(
text.contains("TimeoutError"),
"Should not timeout with Apache Commons Exec fix, but got: "
+ text);
assertTrue(
text.contains("<returncode>0</returncode>"),
"Expected successful execution");
assertTrue(
text.contains("20000"),
"Expected output to contain the last number");
})
.verifyComplete();
}
@Test
@DisplayName("Should handle large file cat without deadlock")
@EnabledOnOs({OS.LINUX, OS.MAC})
void reproduceLargeFileCatDeadlock() throws Exception {
// Create a temporary large file
java.nio.file.Path tempFile = java.nio.file.Files.createTempFile("test_large_", ".txt");
try {
// Write 20KB of data (far exceeds typical pipe buffer size)
StringBuilder content = new StringBuilder();
for (int i = 0; i < 2000; i++) {
content.append("Line ")
.append(i)
.append(": ")
.append("Some test content to fill the buffer\n");
}
java.nio.file.Files.writeString(tempFile, content.toString());
String command = "cat " + tempFile.toString();
// Set a reasonable timeout - should complete quickly with the fix
Mono<ToolResultBlock> result = tool.executeShellCommand(command, 10);
StepVerifier.create(result)
.assertNext(
block -> {
String text = extractText(block);
// Expected: Should complete successfully and return the file
// content
assertFalse(
text.contains("TimeoutError"),
"Should not timeout with Apache Commons Exec fix, but"
+ " got: "
+ text);
assertTrue(
text.contains("<returncode>0</returncode>"),
"Expected successful execution");
assertTrue(
text.contains("Line 1999"),
"Expected output to contain last line");
})
.verifyComplete();
} finally {
java.nio.file.Files.deleteIfExists(tempFile);
}
}
@Test
@DisplayName("Should handle pre-created large test file without deadlock")
@EnabledOnOs({OS.LINUX, OS.MAC})
void reproduceLargeFileCatDeadlockWithTestResource() {
// Use the pre-created test resource file (20KB)
String resourcePath =
getClass().getClassLoader().getResource("large_output_test.txt").getPath();
String command = "cat " + resourcePath;
// Set a reasonable timeout - should complete quickly with the fix
Mono<ToolResultBlock> result = tool.executeShellCommand(command, 10);
StepVerifier.create(result)
.assertNext(
block -> {
String text = extractText(block);
// Expected: Should complete successfully and return the file
// content
assertFalse(
text.contains("TimeoutError"),
"Should not timeout with Apache Commons Exec fix, but got: "
+ text);
assertTrue(
text.contains("<returncode>0</returncode>"),
"Expected successful execution");
assertTrue(
text.contains("This is test content"),
"Expected output to contain file content");
})
.verifyComplete();
}
@Test
@DisplayName("Should handle yes command piped to head without deadlock")
@EnabledOnOs({OS.LINUX, OS.MAC})
void reproducePipeBufferDeadlockWithYesCommand() {
// Generate large output using yes command
// This produces continuous output that will definitely fill the buffer
String command = "yes 'This is a test line with some content' | head -n 1000";
// Set a reasonable timeout - should complete quickly with the fix
Mono<ToolResultBlock> result = tool.executeShellCommand(command, 10);
StepVerifier.create(result)
.assertNext(
block -> {
String text = extractText(block);
// Expected: Should complete successfully without timeout
// PumpStreamHandler consumes output in separate threads
assertFalse(
text.contains("TimeoutError"),
"Should not timeout with Apache Commons Exec fix, but got: "
+ text);
assertTrue(
text.contains("<returncode>0</returncode>"),
"Expected successful execution");
assertTrue(
text.contains("This is a test line"),
"Expected output to contain the repeated line");
})
.verifyComplete();
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agentscope-core/src/main/java/io/agentscope/core/tool/coding/ShellCommandTool.java
Show resolved
Hide resolved
agentscope-core/src/test/java/io/agentscope/core/tool/coding/ShellCommandToolTest.java
Show resolved
Hide resolved
agentscope-core/src/main/java/io/agentscope/core/tool/coding/ShellCommandTool.java
Outdated
Show resolved
Hide resolved
agentscope-core/src/main/java/io/agentscope/core/tool/coding/ShellCommandTool.java
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…pe-ai#619) ## AgentScope-Java Version [The version of AgentScope-Java you are working on, e.g. 1.0.7, check your pom.xml dependency version or run `mvn dependency:tree | grep agentscope-parent:pom`(only mac/linux)] ## Description - Fixed deadlock issue when executing commands with large output (>4-64KB) - Implemented asynchronous stream reading using CompletableFuture to prevent pipe buffer from filling up and blocking child process - Added getOutputWithTimeout() helper method for safe retrieval of async stream reader results with proper timeout and error handling - All existing tests pass without regression close agentscope-ai#617 ## Checklist Please check the following items before code is ready to be reviewed. - [x] Code has been formatted with `mvn spotless:apply` - [x] All tests are passing (`mvn test`) - [x] Javadoc comments are complete and follow project conventions - [x] Related documentation has been updated (e.g. links, examples, etc.) - [x] Code is ready for review
2 participants
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.7, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
pipe buffer from filling up and blocking child process
stream reader results with proper timeout and error handling
close #617
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)