-
-
Notifications
You must be signed in to change notification settings - Fork 560
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
SkipResponseWriter #3537
Merged
raphael
merged 6 commits into
goadesign:v3
from
duckbrain:3522-writerto-skipresponsebodyencodedecode
Jun 20, 2024
Merged
SkipResponseWriter #3537
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6a73729
Implement optional WriterTo path for writing HTTP responses
duckbrain dab2d81
Add comment to the generated interface indicating how to utilize Writ…
duckbrain f0848ee
Fix Deferring unsafe method "Close" on type "io.ReadCloser" according…
duckbrain 96b8e31
Check error for io.Copy in test
duckbrain b235bef
Remove unneeded check for nil pipe reader
duckbrain 0df0530
Merge branch 'v3' into 3522-writerto-skipresponsebodyencodedecode
raphael 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 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 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 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 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,66 @@ | ||
package goa | ||
|
||
import ( | ||
"io" | ||
"sync" | ||
"sync/atomic" | ||
) | ||
|
||
// SkipResponseWriter converts an io.WriterTo into a io.ReadCloser. | ||
// The Read/Close methods this function returns will pipe the Write calls that wt makes, to implement a Reader that has the written bytes. | ||
// If Read is called Close must also be called to avoid leaking memory. | ||
// The returned value implements io.WriterTo as well, so the generated handler will call that instead of the Read method. | ||
// | ||
// Server handlers that use SkipResponseBodyEncodeDecode() io.ReadCloser as a return type. | ||
func SkipResponseWriter(wt io.WriterTo) io.ReadCloser { | ||
return &writerToReaderAdapter{WriterTo: wt} | ||
} | ||
|
||
type writerToReaderAdapter struct { | ||
io.WriterTo | ||
prOnce sync.Once | ||
pr *io.PipeReader | ||
} | ||
|
||
func (a *writerToReaderAdapter) initPipe() { | ||
r, w := io.Pipe() | ||
go func() { | ||
_, err := a.WriteTo(w) | ||
w.CloseWithError(err) | ||
}() | ||
a.pr = r | ||
} | ||
|
||
func (a *writerToReaderAdapter) Read(b []byte) (n int, err error) { | ||
a.prOnce.Do(a.initPipe) | ||
return a.pr.Read(b) | ||
} | ||
|
||
func (a *writerToReaderAdapter) Close() error { | ||
a.prOnce.Do(a.initPipe) | ||
return a.pr.Close() | ||
} | ||
|
||
type writeCounter struct { | ||
io.Writer | ||
n atomic.Int64 | ||
} | ||
|
||
func (wc *writeCounter) Count() int64 { return wc.n.Load() } | ||
func (wc *writeCounter) Write(b []byte) (n int, err error) { | ||
n, err = wc.Writer.Write(b) | ||
wc.n.Add(int64(n)) | ||
return | ||
} | ||
|
||
// WriterToFunc impelments [io.WriterTo]. The io.Writer passed to the function will be wrapped. | ||
type WriterToFunc func(w io.Writer) (err error) | ||
|
||
// WriteTo writes to w. | ||
// | ||
// The value in w is wrapped when passed to fn keeping track of how bytes are written by fn. | ||
func (fn WriterToFunc) WriteTo(w io.Writer) (n int64, err error) { | ||
wc := writeCounter{Writer: w} | ||
err = fn(&wc) | ||
return wc.Count(), err | ||
} |
This file contains 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,49 @@ | ||
package goa | ||
|
||
import ( | ||
"bytes" | ||
"io" | ||
"strings" | ||
"testing" | ||
) | ||
|
||
func TestSkipResponseWriter(t *testing.T) { | ||
const input = "Hello, World!" | ||
var responseWriter io.ReadCloser | ||
|
||
responseWriter = SkipResponseWriter(strings.NewReader(input)) | ||
defer func() { | ||
err := responseWriter.Close() | ||
if err != nil { | ||
t.Error(err) | ||
} | ||
}() | ||
_, ok := responseWriter.(io.WriterTo) | ||
if !ok { | ||
t.Errorf("SkipResponseWriter's result must implement io.WriterTo") | ||
} | ||
|
||
var writerToBuffer bytes.Buffer | ||
_, err := io.Copy(&writerToBuffer, responseWriter) // io.Copy uses WriterTo if implemented | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
if writerToBuffer.String() != input { | ||
t.Errorf("WriteTo: expected=%q actual=%q", input, writerToBuffer.String()) | ||
} | ||
|
||
responseWriter = SkipResponseWriter(strings.NewReader(input)) | ||
defer func() { | ||
err := responseWriter.Close() | ||
if err != nil { | ||
t.Error(err) | ||
} | ||
}() | ||
readBytes, err := io.ReadAll(responseWriter) // io.ReadAll ignores WriterTo and calls Read | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
if string(readBytes) != input { | ||
t.Errorf("Read: expected=%q actual=%q", input, string(readBytes)) | ||
} | ||
} |
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.
I wonder if it might not be better to call
errhandler
here instead of panicking. Typical implementations notify the observability stack of the problem (log, error span, event etc.).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.
I was a bit torn there. Decided to keep the semantics with the non-io.WriterTo version, which also panics. Should probably change both places if it's going to change, but this also feels like a nice bit of code to pull out into a helper for the generated code to call.
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.
Ah good point, hmm might as well stay consistent in both cases and stick with panic then - thanks for bringing this up!