Skip to content

feat: Add Pipe support in process package#1178

Merged
krishankumar01 merged 27 commits intomasterfrom
kkumar-gcc/#567-2
Sep 11, 2025
Merged

feat: Add Pipe support in process package#1178
krishankumar01 merged 27 commits intomasterfrom
kkumar-gcc/#567-2

Conversation

@krishankumar01
Copy link
Member

@krishankumar01 krishankumar01 commented Aug 30, 2025

📑 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

  • Adds process.Pipeline: a fluent builder for pipelines (methods: Input, Env, Path, Timeout, DisableBuffering, Quietly, OnOutput, WithContext).
  • Adds RunningPipe: runtime handle returned by Start(), exposing PIDs(), Running(), Done(), Wait(), Signal() and Stop().
  • Streams per-step live output via OnOutput(key, type, []byte) while still capturing the final step’s stdout/stderr into a Result.

Usage examples

Synchronous (Run) example

package main

import (
	"context"
	"fmt"
	"os"
	"time"

	process "github.com/goravel/framework/process"
	cprocess "github.com/goravel/framework/contracts/process"
)

func main() {
	p := process.NewPipe().
		WithContext(context.Background()).
		Env(map[string]string{"FOO": "bar"}).
		Path("/tmp").
		Timeout(5 * time.Second)

	// Live per-step output
	p.OnOutput(func(key string, typ cprocess.OutputType, line []byte) {
		fmt.Printf("[%s] %s\n", key, string(line))
	})

	builder := func(b cprocess.Pipe) {
		b.Command("printf", "hello\nworld\n").As("producer")
		b.Command("tr", "[:lower:]", "[:upper:]").As("upper")
	}

	result, err := p.Run(builder)
	if err != nil {
		fmt.Println("pipeline failed to start:", err)
		os.Exit(1)
	}

	fmt.Println("exit code:", result.ExitCode())
	fmt.Printf("final stdout:\n%s\n", result.Output())
}

Asynchronous (Start) example

package main

import (
	"fmt"
	"os"
	"os/signal"
	"syscall"
	"time"

	process "github.com/goravel/framework/process"
	cprocess "github.com/goravel/framework/contracts/process"
)

func main() {
	p := process.NewPipe().Quietly() // don't duplicate output to os.Stdout

	p.OnOutput(func(key string, typ cprocess.OutputType, line []byte) {
		fmt.Printf("live [%s]: %s\n", key, string(line))
	})

	builder := func(b cprocess.Pipe) {
		b.Command("bash", "-c", "for i in $(seq 1 100); do echo line-$i; sleep 0.05; done").As("producer")
		b.Command("grep", "line-5").As("filter")
	}

	running, err := p.Start(builder)
	if err != nil {
		fmt.Println("failed to start:", err)
		return
	}

	fmt.Println("pids:", running.PIDs())

	// Wait for Ctrl+C and forward a graceful stop
	interrupt := make(chan os.Signal, 1)
	signal.Notify(interrupt, os.Interrupt, syscall.SIGTERM)

	select {
	case <-interrupt:
		_ = running.Signal(syscall.SIGTERM)
		_ = running.Stop(200 * time.Millisecond, syscall.SIGTERM)
	case <-running.Done():
		// pipeline finished normally
	}

	res := running.Wait()
	fmt.Println("final exit code:", res.ExitCode())
	fmt.Println("final stdout:\n", res.Output())
}

Windows: replace shell commands with PowerShell commands (e.g. powershell -NoLogo -NoProfile -Command "Start-Sleep -Seconds 1; Write-Output 'hello'").

✅ Checks

  • Added test cases for my code

Copilot AI review requested due to automatic review settings August 30, 2025 08:29
@krishankumar01 krishankumar01 requested a review from a team as a code owner August 30, 2025 08:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to Running interface 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.

@krishankumar01 krishankumar01 marked this pull request as draft August 30, 2025 08:50
@codecov
Copy link

codecov bot commented Aug 30, 2025

Codecov Report

❌ Patch coverage is 87.87129% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.86%. Comparing base (b41556f) to head (1c8356b).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
process/process_unix.go 57.89% 18 Missing and 6 partials ⚠️
process/pipeline.go 90.64% 9 Missing and 4 partials ⚠️
process/running_pipe.go 91.75% 6 Missing and 2 partials ⚠️
process/process.go 88.88% 2 Missing and 1 partial ⚠️
process/running.go 98.24% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@krishankumar01 krishankumar01 marked this pull request as ready for review September 1, 2025 05:01
@@ -1,8 +1,60 @@
//go:build windows

// TODO: Revisit process handling for windows, need a better way to handle process using native APIs
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 👍

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

@krishankumar01 krishankumar01 marked this pull request as draft September 9, 2025 06:42
@krishankumar01 krishankumar01 marked this pull request as ready for review September 9, 2025 06:42
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! Thanks

@krishankumar01 krishankumar01 merged commit 123d53e into master Sep 11, 2025
14 checks passed
@krishankumar01 krishankumar01 deleted the kkumar-gcc/#567-2 branch September 11, 2025 08:09
alfanzain pushed a commit to alfanzain/goravel-framework that referenced this pull request Sep 15, 2025
* 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
@krishankumar01 krishankumar01 mentioned this pull request Oct 26, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants