-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add support for non buffered body server responses. #1657
base: master
Are you sure you want to change the base?
Conversation
Fixed test. I included a check in the server as you suggested. |
I changed the approach maintaining the original idea in the API usage. The reason for this change is to support the use of protocols other than HTTP/1.x and to allow compatibility with the packages that provide them (dgrr/http2, for example). If this PR is merged into the code base, I will make the necessary modifications to dgrr/http2. |
// Includes headers and body length. | ||
func (ctx *RequestCtx) BytesSent() int { | ||
return ctx.bytesSent | ||
} |
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.
Why add this function?
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.
After streaming a response, it can be necessary to know the number of bytes sent. That number is known only if we a serving local resources, but unknown if we are proxying from external sources. Bytes sent can represent a cost related to data-transfer. It can be useful for logging and other analysis.
We could return that value in ctx.CloseReponse()
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.
For now I would remove this. This isn't supported with normal responses either so it's weird to only add it for this case now.
Close() error | ||
} | ||
|
||
type UnbufferedWriterHttp1 struct { |
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 don't think this type should be exposed, it can be kept internally. I also wouldn't add Http1
to the name as everything in fastthttp is http1.
func NewUnbufferedWriter(ctx *RequestCtx) *UnbufferedWriterHttp1 { | ||
writer := acquireWriter(ctx) | ||
return &UnbufferedWriterHttp1{ctx: ctx, writer: writer} | ||
} |
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 function can also be kept internal then.
} | ||
|
||
var ErrNotUnbuffered = errors.New("not unbuffered") | ||
var ErrClosedUnbufferedWriter = errors.New("closed unbuffered writer") |
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.
Make it something like use of closed unbuffered writer
@@ -120,6 +120,8 @@ type Response struct { | |||
raddr net.Addr | |||
// Local TCPAddr from concurrently net.Conn | |||
laddr net.Addr | |||
|
|||
headersWritten bool |
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 don't think this state needs to be kept here, you can just add a headersWritten bool
to UnbufferedWriterHttp1
.
h := uw.ctx.Response.Header.Header() | ||
n, err := uw.writer.Write(h) |
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.
Probably faster and more efficient to use uw.ctx.Response.Header.WriteTo(uw.writer)
if !uw.ctx.Response.headersWritten { | ||
// skip body, as we are closing without writing body | ||
uw.ctx.Response.SkipBody = true | ||
_, err := uw.WriteHeaders() | ||
if err != nil { | ||
return fmt.Errorf("error writing headers: %w", err) | ||
} | ||
} |
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.
Can you add a test to make sure this works, calling CloseResponse()
without writing anything to the body.
Adds support for non-buffered responses.
Sometimes we want to respond by copying from a stream (files or remote network connections), but the current method RegisterStreamBody has 2 cons:
This feature allows us to stream directly from the handler context
In server
It will automatically set the Transfer-Encoding header to "chunked" and chunk the content on the fly.