-
-
Notifications
You must be signed in to change notification settings - Fork 559
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
SkipResponseWriter #3537
Conversation
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 is really good! I left a couple of comments.
if f, ok := w.(http.Flusher); ok { | ||
f.Flush() | ||
} | ||
panic(http.ErrAbortHandler) // too late to write an error |
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.
if wt, ok := o.Body.(io.WriterTo); ok {
n, err := wt.WriteTo(w)
if err != nil {
if n == 0 {
if err := encodeError(ctx, w, err); err != nil {
errhandler(ctx, w, err)
}
} else {
if f, ok := w.(http.Flusher); ok {
f.Flush()
}
panic(http.ErrAbortHandler) // too late to write an error
}
}
return
}
// handle immediate read error like a returned error
buf := bufio.NewReader(o.Body)
if _, err := buf.Peek(1); err != nil && err != io.EOF {
if err := encodeError(ctx, w, err); err != nil {
errhandler(ctx, w, err)
}
return
}
if err := encodeResponse(ctx, w, o.Result); err != nil {
errhandler(ctx, w, err)
return
}
if _, err := io.Copy(w, buf); err != nil {
if f, ok := w.(http.Flusher); ok {
f.Flush()
}
panic(http.ErrAbortHandler) // too late to write an error
}
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!
pkg/skip_response_writer.go
Outdated
} | ||
|
||
func (a *writerToReaderAdapter) initPipe() { | ||
if a.pr != nil { |
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.
Is this test necessary?
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.
Nope, that's a holdover from before switching to sync.Once.
Thank you for the great contribution! I was wondering if you'd be up to updating the upload_download example to take advantage of this new feature after we release it? |
Implementation for #3522
Since the tests, don't include the comment, here's the result:
Note: Tried this out in one of my projects and added
WriterToFunc
to the exported API as well, since I found myself wanting it for most of the handlers I was rewriting. Allows you to use a closure and avoids needing to manually keep track of bytes written, while still preserving theio.WriterTo
interface. Useful for times when you're passing the writer to something else that doesn't report how many bytes are written likejson.Encoder
orxml.Encoder
.