-
Notifications
You must be signed in to change notification settings - Fork 7
go-pipe v2: Stage API, pooled copies, MemoryWatch removal #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
znull
wants to merge
95
commits into
main
Choose a base branch
from
znull/stage-v2-clean
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 92 commits
Commits
Show all changes
95 commits
Select commit
Hold shift + click to select a range
c4146f4
Shush the linter
mhagger de3225f
pipeline_test.go: remove unnecessary tmpdirs and simplify test setup
mhagger 9fd5b2c
Add some benchmarks of moving a bunch of data through a pipeline
mhagger ca78f76
Simplify the `NopCloser`s
mhagger 2f59602
Stage: change the interface to make stdin/stdout handling more flexible
mhagger cc9cd67
Add some tests that `Pipeline.Start()` picks the right stdin/stdout
mhagger 6c6dfe5
Port MemoryLimitWithObserver to new Stage interface
znull cd47557
Restore panic handler for Function stages
znull 48dffbd
Fix memoryWatchStage.Wait() to always call stopWatching()
znull 92ad57c
Fix lint errors
znull 6a0abf3
Restore identity-copy behavior for empty pipelines
znull 9606368
Add tests pinning command stdout fd-pass fast path
znull 3a1f8d8
Pool buffers for the non-*os.File command stdout copy path
znull 46f19e4
Forward panic handler through wrapper stages
znull 121ae94
Avoid leaking pooled-stdout goroutine when cmd.Start() fails
znull bf5820b
Bump module path to v2 for breaking Stage interface change
znull 6102173
remove IOPreferenceNil
znull 754784b
properly handle nil input/output
znull 9dca23e
allow earlier GC of lateClosers
znull 9ee30b1
export Unwrap{Reader,Writer}; unwrap goStage stdin fully
znull 75797b9
rewrite cleanup/unwind in forward order
znull 512c345
Thread panic handler through Start; drop StagePanicHandlerAware
znull 4278c36
Recover panics escaping the memory-watch goroutine
znull 9b53532
update README for v2 bump
znull f893837
Replace the MemoryLimit constructor trio with MemoryWatch + options
znull f05b5c6
memoryWatcher: new helper type
mhagger 2cc87f4
memoryWatcher: add some methods
mhagger ebdc8f5
memoryWatcher.watch(): simplify loop termination
mhagger 4ab48c3
memoryWatcher.update(): new method
mhagger 8a96683
memoryWatcher.stage: new field
mhagger d49ce65
memoryWatchConfig: subsume type into `memoryWatcher`
mhagger 0e8133e
memoryWatchStage.monitor(): only recover if there's a panic handler
mhagger 939002c
memoryWatcher.watch(): defer Ticker.Stop()
znull 099bd87
Test memory-watch panic handling via a booby-trapped GetRSSAnon
znull 9967cc9
Test no-handler panic propagation without a subprocess
znull e20065d
TestMemoryLimitKillsEvenIfEventHandlerPanics: use constructor
mhagger 624322b
memoryWatcher: inline type into `memoryWatchStage`
mhagger f3d400e
memoryWatchStage: rename all method receivers to `m`
mhagger 073178c
memoryWatchStage: reorder method definitions more thematically
mhagger 418316c
Merge pull request #54 from github/split/memorywatch-api
znull c398b7b
Decouple close-responsibility from stdin/stdout dynamic type
znull df1841a
Consolidate Env + StartOptions into a single StageOptions
znull 2e71004
Split Stage Start streams from closers
znull 0fed40d
Replace stage IO preferences with requirements
znull df7ed3e
Allow stages to declare that stdin or stdout must be absent.
znull 24e9427
Let Function stages declare stream-presence requirements.
znull 4675a07
Use bools for Stage stream ownership
znull 7c0898b
Merge pull request #53 from github/split/close-decouple
znull be3cf9d
Remove MemoryWatch from go-pipe
znull 5af021e
Add stage-scoped environment wrapper
znull ffcc348
Fix stage env option slice aliasing
znull f89e773
env_stage.go: Preserve command hooks through stage env wrapper
znull 1ae6d3b
lint fix: copyloopvar
znull 145c720
Normalize env stage tests
znull 5538f2a
Merge pull request #56 from github/znull/remove-memorywatch
znull a42eaea
we don't need both processProvider and processKiller
znull fa89a40
convert tests to testify
znull bcfadc2
PR feedback: comment update
znull 75b65f5
don't allocate more space than needed in stage slice
znull 0a7d303
make checkStreamRequirement* helpers into methods
znull e02a46a
only loop once while populating stage slice
znull cf0cb9d
pacify linter
znull dfdc413
partially restore WithStagePanicHandler comment
znull a327b2f
Make commandStage close stdout streams if pooled stdout setup fails
znull f040a53
Close stdout streams when startup fails before the final stage starts
znull 08d5b4b
Introduce stream types
znull c6d12cb
windows could have these, we don't know until we try
znull 1d7a969
test Go producer behavior after downstream early exit
znull 33a4e28
document producer pipe errors in v2 migration
znull ac585a8
fully document stream ownership (close responsibility)
znull a65fafb
Document early-close producer caveats
znull 433e104
Align start-failure cleanup with stream ownership
znull 642eb3e
InputStream, OutputStream: move types to a separate file
mhagger 67ca961
InputStream, OutputStream: change `Close()` methods to pass errors th…
mhagger e3a8223
Stage: update docstring to reflect use of streams
mhagger aa3db90
InputStream, OutputStream: remove the `Closer()` methods
mhagger 63d8e51
commandStage.Start(): rename some local variables
mhagger 6ef82b5
InputStream, OutputStream: make these into pointer types
mhagger 29edba3
InputStream, OutputStream: make `Close()` idempotent
mhagger 8466274
InputStream, OutputStream: improve docstrings
mhagger b30a165
Pipeline: use `InputStream` and `OutputStream`
mhagger 07cbbcf
Stage: improve docstring
mhagger e772ff0
StreamRequirement: move to separate file
mhagger fbe470e
StreamRequirement: encode also whether the stream has to be nil
mhagger cb3bf1d
StreamRequirement.Validate(): make method public
mhagger 8e22ddc
stageJoiner: new helper type for creating pipes between stages
mhagger 42a4910
Stage: do some more work on the docstring
mhagger f2d53ab
stageJoiner: cache each stage's Requirements()
znull 7072f01
Pipeline.Start: validate stream requirements before creating pipes
znull a274f74
Merge pull request #58 from github/moar-streams
mhagger a86cf5f
Merge pull request #59 from github/stage-joiner
mhagger f8adc48
Merge pull request #57 from github/znull/v2-early-close-regression-tests
znull 9dacf4b
fix comment typo
znull 3d273c2
use correct code example in README
znull 2de883d
pipeline: add missing closePipes() on validation error
znull File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,34 @@ | ||
| # go-pipe [](https://pkg.go.dev/github.com/github/go-pipe) | ||
| # go-pipe [](https://pkg.go.dev/github.com/github/go-pipe/v2) | ||
| A package used to easily build command pipelines in your Go applications | ||
|
|
||
| # Important | ||
| We have not thoroughly tested this package on OSs other than Linux, especially Windows. At this time, using this package on Windows based systems is considered experimental and will be supported only on a best effort basis. | ||
|
|
||
| # Migrating to v2 | ||
|
|
||
| It's normal for pipelines to stop before all input has been consumed[^1]. If an earlier stage continues writing after that happens, the write side of the pipe can fail with `EPIPE`, `SIGPIPE`, or `io.ErrClosedPipe`. | ||
|
|
||
| In go-pipe v1 it was possible to get away without handling this case, because a command stage's stdin was connected in a way that often (but not necessarily!) drained the write side and hid the error from the previous stage feeding it. That was an implementation detail, not a guarantee. In go-pipe v2, producer stages are more likely to be connected directly to a command's stdin, and thus see the error themselves. | ||
|
|
||
| Fortunately, this is easily handled by wrapping the stage with `pipe.IgnoreError(stage, IsPipeError)`. If the producer only writes output and is otherwise stateless, that's the only thing needed. | ||
|
|
||
| If the producer also updates state, metrics, cursors, or has other side effects, in a way that depends on how much of the output was produced, then in addition to using `pipe.IgnoreError`, you must also ensure producer-owned state is brought to a consistent point before returning the error. | ||
|
|
||
| For example, if a stateful producer function must process its entire input for correctness regardless of whether it was read by the consumer, it should use a pattern like: | ||
|
|
||
| ```go | ||
| var writeErr error | ||
| for _, item := range items { | ||
| updateState(item) | ||
| if writeErr == nil { | ||
| _, writeErr = fmt.Fprintln(stdout, item) | ||
| } | ||
| } | ||
| return writeErr | ||
| ``` | ||
|
|
||
| # Links | ||
|
|
||
| * [Docs](https://pkg.go.dev/github.com/github/go-pipe) | ||
| * [Docs](https://pkg.go.dev/github.com/github/go-pipe/v2) | ||
|
|
||
| [^1]: In `cat foo | head | grep -q`, for example, either `head` or `grep` could exit before its input is fully consumed. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| module github.com/github/go-pipe | ||
| module github.com/github/go-pipe/v2 | ||
|
|
||
| go 1.24.0 | ||
|
|
||
|
|
||
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.