Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pablolagos
Copy link

@pablolagos pablolagos commented Nov 11, 2023

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:

  • Is executed after the handler function returns
  • Create 2 go threads and 2 channels for each response

This feature allows us to stream directly from the handler context

In server

´func handle(ctx *fasthttp.RequestCtx) {
     // DisableBuffering modifies fasthttp to disable headers and body buffering for this request
     ctx.DisableBuffering()
     
     // Write headers
     ctx.Response.Header.Set(.....)
     ctx.Response.Header.Set(.....)
     ctx.Response.Header.Set(.....)
     ctx.SetStatusCode(StatusOK)
    
     // Write body. Headers will be sent and cannot be modified once the body begins to be dispatched.
     // src can be any io.Reader or io.ReaderFrom or any object implementing io.WriteTo
     // The process would be unbuffered, well, actually using at most a 32KB buffer 
     // according to the current io.Copy implementation.
     // Ideal for serving large files without storing them on RAM. 
     io.Copy(ctx, src)

     // Optionally we can call CLoseReponse() or it will be called automatically after handler returns
     ctx.CloseResponse()

     fmt.Printf("Total bytes sent including headers: %d",ctx.BytesSent())
}

It will automatically set the Transfer-Encoding header to "chunked" and chunk the content on the fly.

server_test.go Show resolved Hide resolved
server_test.go Show resolved Hide resolved
@pablolagos
Copy link
Author

Fixed test. I included a check in the server as you suggested.

@pablolagos
Copy link
Author

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
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this function?

Copy link
Author

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()

Copy link
Collaborator

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 {
Copy link
Collaborator

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}
}
Copy link
Collaborator

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")
Copy link
Collaborator

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
Copy link
Collaborator

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.

Comment on lines +79 to +80
h := uw.ctx.Response.Header.Header()
n, err := uw.writer.Write(h)
Copy link
Collaborator

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)

Comment on lines +96 to +103
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)
}
}
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants