Make the stream types more capable and use them in more places#58
Open
mhagger wants to merge 9 commits into
Open
Make the stream types more capable and use them in more places#58mhagger wants to merge 9 commits into
mhagger wants to merge 9 commits into
Conversation
This is to force all close calls to come through `InputStream.Close()` and `OutputStream.Close()`, which will soon get some more functionality.
This will be needed in a moment.
Ignore all but the first call to `Close()`.
In the type comments, explain why these types don't implement `io.Reader` and `io.Writer`. Otherwise, some helpful person is sure to come along and add `Read()` and `Write()` methods, to the detriment of performance and even changing some semantics.
Change the types of some `Pipeline` fields: * `stdin` to `InputStream` * `stdout` to `OutputStream` That way we don't have to manage their closers separately.
There was a problem hiding this comment.
Pull request overview
This PR evolves the stream ownership API by moving InputStream/OutputStream into dedicated types with idempotent Close() error, switching them to pointer semantics (including allowing nil stream pointers), and wiring these types through the Stage contract and Pipeline plumbing so stream closing behavior is consistently centralized in Close().
Changes:
- Introduces
pipe.InputStream/pipe.OutputStreampointer types with idempotentClose() errorand nil-safe behavior. - Updates
Stage.Startand all stage implementations/tests to accept*InputStream/*OutputStreamand useClose()instead ofCloser(). - Refactors
Pipelineto store stdin/stdout as stream types and pass them through stage wiring/abort paths.
Show a summary per file
| File | Description |
|---|---|
| pipe/streams.go | Adds new pointer-based InputStream/OutputStream types with idempotent Close() error. |
| pipe/stage.go | Updates Stage.Start signature and ownership docs/examples for stream pointers and Close(). |
| pipe/pipeline.go | Refactors pipeline stdin/stdout fields and stage wiring to use stream types throughout. |
| pipe/pipeline_test.go | Updates test stages to use pointer stream parameters and Close() error. |
| pipe/pipe_matching_test.go | Updates pipe sniffing test stage to the new stream pointer API. |
| pipe/function.go | Updates Go-function stage wiring to close via stream Close() and propagate close errors. |
| pipe/env_stage.go | Updates env-wrapping stage to the new stream pointer API. |
| pipe/command.go | Updates command stage to use stream pointers and rely on stream Close() for ownership. |
| pipe/command_stdout_fastpath_test.go | Adjusts fastpath tests to construct pointer-based output streams. |
| pipe/close_responsibility_test.go | Updates close ownership tests to validate idempotent close behavior and counts. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pipe/command.go:122
closeEarlyClosersignores errors fromClose(). With streams now returning errors (andWait()already propagating late-closer errors), this silently drops failures when closing our copy of stdin/stdout aftercmd.Start(). Consider capturing the first close error and surfacing it fromWait()whencmd.Wait()succeeds, similar to howcloseLateClosers()is handled.
closeEarlyClosers := func() {
for _, closer := range earlyClosers {
_ = closer.Close()
}
}
- Files reviewed: 10/10 changed files
- Comments generated: 4
| type InputStream struct { | ||
| reader io.Reader | ||
|
|
||
| // once is used to ensure that `Close() is only called once. |
| type OutputStream struct { | ||
| writer io.Writer | ||
|
|
||
| // once is used to ensure that `Close() is only called once. |
Comment on lines
+47
to
50
| // cmd.Stdin = stdin.Reader() // Similarly for stdout | ||
| // cmd.Start(…) | ||
| // f.Close() // close our copy | ||
| // stdin.Close() // Close our copy | ||
| // cmd.Wait() |
Comment on lines
+59
to
+62
| // cmd.Stdin = stdin.Reader() // Similarly for stdout | ||
| // cmd.Start(…) | ||
| // cmd.Wait() | ||
| // r.Close() | ||
| // stdin.Close() // Close |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Now that we have the
InputStreamandOutputStreamtypes, we might as well use them in more places and pin some more functionality on them:Close()to pass any errors through.Closer()methods to force the streams'Close()methods to be used instead. This allows us to put more functionality inClose(), like…Close()method idempotent. To make this possible, we first need to:InputStreamandOutputStreaminto pointer types.InputStreamandOutputStreamin the definition ofPipeline.The commits are written to be readable one by one, and CI passes after each commit. This PR is against the branch from PR #57.
The next planned step
Changing the streams to pointer types is interesting when starting up stages: once you have successfully passed a stream to
Stage.Start(), you can clear your own pointer and then you don't have to worry about accidentally callingClose()on a stream that is already owned by a stage. That, together with the fact that it is allowed to callClose()onnilpointers, will make the bookkeeping trivial. I've got some commits that change the helper class used byPipeline.Start()and will try to push that soon. On the other hand, it's a pointer so it's visually a little bit more noisy.The idempotency of
Close()might be a little bit controversial. Maybe there should be an error if the client callsClose()twice, if we want the client to be forced to fix whatever's wrong? On the other hand, it would letPipelineoffer the service that it callsClose()on all streams when the corresponding stage finishes, in case the client forgot to do so, to add a little bit of belt 'n' suspenders. I'm open to being convinced that this is a bad idea./cc @znull