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

fasthttp.Response does not handle Content-Type: text/event-stream properly #1129

Closed
ShivanshVij opened this issue Oct 20, 2021 · 11 comments
Closed

Comments

@ShivanshVij
Copy link
Contributor

If there's a webserver that FastHTTP is attempting to read from using the following:

err = ctx.Response.Header.Read(bufferedNetConn)
if err != nil {
	ctx.SetConnectionClose()
	ctx.SetStatusCode(http.StatusBadGateway)
	_ = netConn.Close()
	return
}

Then then code will hang indefinitely if the webserver is attempting to stream data using the text/event-stream content-type.

A potential solution is to go into the fasthttp.readBody function, check if the resp.Header for the text/event-stream content-type and if it's there, then we simply stream it using the BodyStreamWriter function.

Alternatively, we could also just make the resp.bodyBuffer and fashttp.readBody functions exported, and users could check the headers and decide whether to stream or not themselves.

@ShivanshVij
Copy link
Contributor Author

ShivanshVij commented Oct 20, 2021

A working BodyStreamWriter function could look like this (I've tested this and it works):

ctx.SetBodyStreamWriter(func(w *bufio.Writer) {
  buf := pbytes.GetLen(defaults.MaxBodySize) // ignore the pbytes, it's basically a byte pool so we can do zero-copy
  for {
	  n, err := io.CopyBuffer(w, chunkedReader, buf)
	  if n > 0 {
		  _ = w.Flush()
	  }
	  if err != nil {
		  if !errors.Is(err, io.EOF) {
			  _ = netConn.Close()
		  }
		  break
	  }
  }
  pbytes.Put(buf)
})

@erikdubbelboer
Copy link
Collaborator

#998 (comment)

@ShivanshVij
Copy link
Contributor Author

I'm not talking about the FastHTTP client, I'm talking about the FastHTTP server.

Is this a use case that FastHTTP just won't support?

@erikdubbelboer
Copy link
Collaborator

Can you explain better what you mean? Do you want your fasthttp server to respond with text/event-stream (like #422 (comment)), or is the client sending your fasthttp server a text/event-stream?

@ShivanshVij
Copy link
Contributor Author

We're trying to use FastHTTP as a reverse proxy, so it's acting as both the client and the server.

It basically needs to be able to forward the text-stream, and if we default to always using the bodystreamwriter then performance takes a major hit.

@ShivanshVij
Copy link
Contributor Author

Considering the scope of this change is extremely small, I'm happy to submit a PR if that works. It would allow FastHTTP to support a much wider set of use cases as well.

@erikdubbelboer
Copy link
Collaborator

Sure you can always submit a PR, I'm curious what change you have in mind.

@ShivanshVij
Copy link
Contributor Author

Cool, I've added the PR. It's very straight forward and does not functionally change FastHTTP - but does unblock our team.

@ShivanshVij
Copy link
Contributor Author

ShivanshVij commented Oct 20, 2021

Now we can simply do this:

ctx.Response.SkipBody = true
err = ctx.Response.Read(bufEgress)
if err != nil {
	log.Logger.Debug().Err(err).Msgf("error in Response Read for %s during FastHTTP Proxy", ctx.RequestURI())
	ctx.Response.SetStatusCode(http.StatusInternalServerError)
	ctx.SetConnectionClose()
	_ = egress.Close()
	return
}
ctx.Response.SkipBody = false

DeleteResponseHopHeaders(&ctx.Response)

err = ctx.Response.ReadBody(bufEgress, 0)
if err != nil {
	log.Logger.Debug().Err(err).Msgf("error in Response Read for %s during FastHTTP Proxy", ctx.RequestURI())
	ctx.Response.SetStatusCode(http.StatusInternalServerError)
	ctx.SetConnectionClose()
	_ = egress.Close()
	return
}

Of course this example is missing the check that sees if the contentType == text/event-stream

@erikdubbelboer
Copy link
Collaborator

I'm still a little bit confused about your example. Is ctx here a fasthttp.RequestCtx or are you doing your own connection handling as it seems? So you're not using fasthttp as server but you're only using fasthttp.Request and fasthttp.Response and that's why you want these methods?

@ShivanshVij
Copy link
Contributor Author

Yes, the ctx is the fasthttp.RequestCtx, and I'm doing my own connection handling but passing it into fasthttp using fasthttp.ServeConn.

I guess in this way we're using fasthttp as the server. One of the main reasons for this is HTTP2/3 support. It's not available in fasthttp (which makes sense), but for clients that don't support HTTP2/HTTP3 we need to have a performant option - and fasthttp is the most performant option.

So we check if the client supports HTTP2/3 and if it doesn't then we pass it to the fasthttp.ServeConn function with a handler that looks something like the above.

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

No branches or pull requests

2 participants