feat: Add Pipe support in process package#1178
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for a DisableBuffering() method to the process package and introduces a Done() channel to the Running interface for non-blocking process monitoring. The changes enable better memory management for processes that produce large output volumes and provide a more efficient way to wait for process completion.
- Added
DisableBuffering()method to prevent stdout/stderr buffering in memory - Added
Done()channel toRunninginterface for non-blocking process completion monitoring - Refactored process waiting mechanism from sync.Once to goroutine-based approach
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| contracts/process/process.go | Added DisableBuffering method to Process interface with documentation |
| contracts/process/running.go | Added Done method to Running interface and updated documentation |
| process/process.go | Implemented DisableBuffering functionality with conditional buffer creation |
| process/running.go | Added Done channel and refactored Wait mechanism to use goroutine |
| mocks/process/Process.go | Added mock implementation for DisableBuffering method |
| mocks/process/Running.go | Added mock implementation for Done method |
| process/process_unix_test.go | Added test case for DisableBuffering functionality |
| process/process_windows_test.go | Added test case for DisableBuffering functionality |
| process/running_unix_test.go | Refactored tests and added comprehensive Done channel and Stop method tests |
| process/running_windows_test.go | Refactored tests and added comprehensive Done channel and Stop method tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1178 +/- ##
==========================================
+ Coverage 68.29% 68.86% +0.57%
==========================================
Files 233 235 +2
Lines 15071 15386 +315
==========================================
+ Hits 10292 10596 +304
- Misses 4399 4406 +7
- Partials 380 384 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
process/process_windows.go
Outdated
| @@ -1,8 +1,60 @@ | |||
| //go:build windows | |||
|
|
|||
| // TODO: Revisit process handling for windows, need a better way to handle process using native APIs | |||
There was a problem hiding this comment.
Currently, I’m testing only on Unix-like systems and doing basic testing for Windows through CI/CD. I’ll need to optimize the Windows-specific logic on an actual Windows machine, since CI/CD involves a lot of back-and-forth. For now, the Windows implementation is kept very basic.
There was a problem hiding this comment.
I suggest that the windows should be implemented and tested in this PR. If it's required, I can buy a remote windows server for the test.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 5942827 | Previous: b41556f | Ratio |
|---|---|---|---|
BenchmarkFile_ReadWrite |
304920 ns/op 6257 B/op 99 allocs/op |
192975 ns/op 6258 B/op 99 allocs/op |
1.58 |
BenchmarkFile_ReadWrite - ns/op |
304920 ns/op |
192975 ns/op |
1.58 |
This comment was automatically generated by workflow using github-action-benchmark.
* add `DisableBuffering` and `Done` methods
* update output writer to accept both process and pipe OnOutput
* move unexported platform process specific methods in running_{platform}
* move unexported platform process specific methods in running_{platform}
* fix test cases for windows
* fix test cases for windows
* fix test cases for windows
* move all process related function for process_{platform}
* push a draft implementation of process pipe
* close pipe writer after process finishes
* fix unix test cases
* call cancel method on error when timeout is given
* add pipe test cases for unix like os
* add test cases for windows
* optimize pipe
* handle panic recovers
* add godoc comments
* add sysProc for windows
* expose process.Step as interface
* fix test cases
* update go mod
* optimize test
* add mockery as tool
* fix test
* fix window tests
* fix windows test cases
📑 Description
Related to #1171, goravel/goravel#567
Add process pipelining support (builder-style Pipe and runtime RunningPipe) so multiple commands can be connected stdout -> stdin, stream per-step output, and be controlled as a single unit.
What this PR does
process.Pipeline: a fluent builder for pipelines (methods:Input,Env,Path,Timeout,DisableBuffering,Quietly,OnOutput,WithContext).RunningPipe: runtime handle returned byStart(), exposingPIDs(),Running(),Done(),Wait(),Signal()andStop().OnOutput(key, type, []byte)while still capturing the final step’s stdout/stderr into aResult.Usage examples
Synchronous (
Run) exampleAsynchronous (
Start) example✅ Checks