-
Notifications
You must be signed in to change notification settings - Fork 7
Enable sendfile for network destinations; fix pool bypass on Go 1.26+ #50
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
22c71ee
Add test for pool buffer bypass via *os.File WriterTo
znull c5839cb
Fix pool buffer bypass caused by *os.File WriterTo
znull afe1947
Add test for ReaderFrom hidden by nopWriteCloser
znull 6e7976a
Unwrap nopWriteCloser to expose ReaderFrom
znull 60bd6c3
Fix lint errors in ioCopier tests
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
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 |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| package pipe | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "io" | ||
| "os" | ||
| "runtime" | ||
| "sync/atomic" | ||
| "testing" | ||
| ) | ||
|
|
||
| // TestIOCopierPoolBufferUsed verifies that ioCopier uses the sync.Pool | ||
| // buffer rather than allocating a fresh one. On Go 1.26+, *os.File | ||
| // implements WriterTo, which causes io.CopyBuffer to bypass the | ||
| // provided pool buffer entirely. Instead, File.WriteTo → | ||
| // genericWriteTo → io.Copy allocates a fresh 32KB buffer on every call. | ||
| func TestIOCopierPoolBufferUsed(t *testing.T) { | ||
| const payload = "hello from pipe\n" | ||
|
|
||
| // Pre-warm the pool so Get doesn't allocate. | ||
| copyBufPool.Put(copyBufPool.New()) | ||
|
|
||
| // Warm up: run once to stabilize lazy init. | ||
| pr, pw, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| go func() { | ||
| _, _ = pw.Write([]byte(payload)) | ||
| pw.Close() | ||
| }() | ||
| var warmBuf bytes.Buffer | ||
| c := newIOCopier(nopWriteCloser{&warmBuf}) | ||
| _, _ = c.Start(context.TODO(), Env{}, pr) | ||
| _ = c.Wait() | ||
|
|
||
| // Now measure: run the copy and check how many bytes were allocated. | ||
| // If the pool buffer is bypassed, a fresh 32KB buffer is allocated. | ||
| pr, pw, err = os.Pipe() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| go func() { | ||
| _, _ = pw.Write([]byte(payload)) | ||
| pw.Close() | ||
| }() | ||
| var buf bytes.Buffer | ||
| c = newIOCopier(nopWriteCloser{&buf}) | ||
|
|
||
| // GC clears sync.Pool, so re-warm it afterward to isolate the | ||
| // measurement from pool repopulation overhead. | ||
| runtime.GC() | ||
| copyBufPool.Put(copyBufPool.New()) | ||
|
|
||
| var m1, m2 runtime.MemStats | ||
| runtime.ReadMemStats(&m1) | ||
|
|
||
| _, _ = c.Start(context.TODO(), Env{}, pr) | ||
| _ = c.Wait() | ||
|
|
||
| runtime.GC() | ||
| runtime.ReadMemStats(&m2) | ||
|
|
||
| if buf.String() != payload { | ||
| t.Fatalf("unexpected output: %q", buf.String()) | ||
| } | ||
|
|
||
| allocBytes := m2.TotalAlloc - m1.TotalAlloc | ||
| // A bypassed pool buffer causes ~32KB of allocation. | ||
| // With the pool buffer working, we expect well under 32KB. | ||
| const maxBytes = 16 * 1024 | ||
| if allocBytes > maxBytes { | ||
| t.Errorf("ioCopier allocated %d bytes during copy (max %d); "+ | ||
| "pool buffer may be bypassed by *os.File WriterTo", | ||
| allocBytes, maxBytes) | ||
| } | ||
| } | ||
|
|
||
| // readFromWriter is a test writer that implements io.ReaderFrom and | ||
| // records whether ReadFrom was called. | ||
| type readFromWriter struct { | ||
| bytes.Buffer | ||
| readFromCalled atomic.Bool | ||
| } | ||
|
|
||
| func (w *readFromWriter) ReadFrom(r io.Reader) (int64, error) { | ||
| w.readFromCalled.Store(true) | ||
| return w.Buffer.ReadFrom(r) | ||
| } | ||
|
|
||
| func (w *readFromWriter) Close() error { return nil } | ||
|
|
||
| // TestIOCopierUsesReadFrom verifies that ioCopier dispatches to | ||
| // ReaderFrom when the destination writer supports it, even when | ||
| // wrapped in nopWriteCloser (as happens with WithStdout). | ||
| func TestIOCopierUsesReadFrom(t *testing.T) { | ||
| const payload = "hello readfrom\n" | ||
|
|
||
| pr, pw, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| go func() { | ||
| _, _ = pw.Write([]byte(payload)) | ||
| pw.Close() | ||
| }() | ||
|
|
||
| w := &readFromWriter{} | ||
| c := newIOCopier(nopWriteCloser{w}) | ||
| _, _ = c.Start(context.TODO(), Env{}, pr) | ||
| _ = c.Wait() | ||
|
|
||
| if w.String() != payload { | ||
| t.Fatalf("unexpected output: %q", w.String()) | ||
| } | ||
|
|
||
| if !w.readFromCalled.Load() { | ||
| t.Error("ioCopier did not call ReadFrom on destination; " + | ||
| "nopWriteCloser may be hiding the ReaderFrom interface") | ||
| } | ||
| } | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is meant to guard against the Go 1.26+ *os.File WriterTo path, but the module targets Go 1.24 (go.mod) where *os.File likely doesn't implement io.WriterTo, so the failure mode may not be exercised in CI. Consider making the test version-independent (e.g., use a custom reader that implements io.WriterTo and would allocate/counter-increment if WriteTo is invoked, or explicitly skip unless the pipe reader implements io.WriterTo).