Content-Length not set in ForwardResponseMessage

[joshgarnett]

Content-Length is not set before calling w.Write at https://github.com/grpc-ecosystem/grpc-gateway/blob/main/runtime/handler.go#L179. This ends up relying on the default Go behavior, which will only set Content-Length for small responses. When using CDNs like CloudFront they will not compress the origin responses unless Content-Length is set.

In theory the header could be added by a WithForwardResponseOption, the downside being that the message needs to be serialized twice once by the WithForwardResponseOption and once by the ForwardResponseMessage code. That said it could be incorrect if if rb, ok := resp.(responseBody); ok { a few lines up is true.

Is there a better way to do this? Thanks in advance!

[johanbrandhorst]

Thanks for your issue! I'd like to ask some clarifying questions:

1. Could you elaborate on the behavior of the net/http handler in this case? If they choose to only do this for "small responses", why should we do anything differently?
2. Is this something that could be by a custom middleware instead? I'm thinking something that wraps the http.ResponseWriter and counts the bytes written, setting the header before writing it to the wire. That would mean we wouldn't have to add it as an option here.
3. Thanks for the PR, but please refrain from submitting anything until we've discussed the best way forward.

[joshgarnett]

Hey @johanbrandhorst,

On question 1:

From https://github.com/golang/go/blob/960fa9bf66139e535d89934f56ae20a0e679e203/src/net/http/server.go#L125

	// If [ResponseWriter.WriteHeader] has not yet been called, Write calls
	// WriteHeader(http.StatusOK) before writing the data. If the Header
	// does not contain a Content-Type line, Write adds a Content-Type set
	// to the result of passing the initial 512 bytes of written data to
	// [DetectContentType]. Additionally, if the total size of all written
	// data is under a few KB and there are no Flush calls, the
	// Content-Length header is added automatically.

https://github.com/golang/go/blob/960fa9bf66139e535d89934f56ae20a0e679e203/src/net/http/server.go#L1582 also has some additional comments.

The library doesn't know if the calling code is going to call write once or multiple times. In this case, we know the Write method is only called once as long as doForwardTrailers isn't set.

On question 2:

I'm not sure of a way this can be done without writing the data to a buffer first, which adds additional allocations. This could be problematic for very large responses.

[johanbrandhorst]

Thanks for your responses. I wonder if we can't just enable this behavior by default since we know the size of the buffer we want to write. Worst case it's going to add a content-length header where we didn't before, but that seems like a better experience. What do you think? Feel free to update the PR and remove the option if you agree.

[joshgarnett]

I'm supportive of that, the only edge case I could think of is if a user already had some other middleware that was writing the header and maybe causing some trouble there. I'll update the PR now.

[joshgarnett]

I recreated the PR with a cleaner history

[seinshah]

Hi. This introduced a bug in my code after upgrading to the newer version.

In my code base, I apply an on-the-fly gzip compression of the response on a higher level where the gzip writer is passed as the response writer. I was relying on the Go's automatically added Content-Length header for this.

With the following changes, Content-Length is set by the gateway internally and when gzip response is being written, I often receive http: wrote more than the declared Content-Length. This happens when the response is empty (i.e. {}) and gateway adds the header Content-Length: 2, but gzip compressed response is bigger as it needs to add at least 10 bytes for its header.

grpc-gateway/runtime/handler.go

Lines 193 to 195 in e99e853

	if !doForwardTrailers {
	w.Header().Set("Content-Length", strconv.Itoa(len(buf)))
	}

May I know if we can plan anything to address that on the gateway level? Something like either enable or disable this new behavior using a feature flag?

Although in my case, it happened for an emptypb response, it still may happen for any type of on-the-fly response modification.

[johanbrandhorst]

Hm, thanks for your report, that is unfortunate. As I see it though, you could still work around this in your code with a http.ResponseWriter wrapper that intercepts the setting of the Content-Type header, which would allow you to set that header in your gzip middleware instead. As long as there is some workaround, I think I'd prefer not to introduce other options. Can you see if that fixes your issue?

[seinshah]

Yes. I can intercept this on my side and fix the issue for my case.
Mostly wanted to point it out as more people may face the same breaking change by updating grpc-gateway package if they're using any on-the-fly modification. We could prevent others from facing the same issue in future by making Content-Length header as an opt-in option.
I'll fix my case in any case.

[choyri]

I have a service that uses gzip and it encountered the same problem. This breaking change is really frustrating.

[johanbrandhorst]

Thanks for your report. I'm wondering if we should revert this change. It was not made with the knowledge that it would break users. You are the second person to report this, which means there are probably hundreds out there that will be broken by this change. @joshgarnett would you mind opening a PR that enables this through an option instead of by default?

[joshgarnett]

I'll take a look at this early next week. Stay tuned.

…

On Fri, Dec 6, 2024 at 1:31 AM Johan Brandhorst-Satzkorn < ***@***.***> wrote: Thanks for your report. I'm wondering if we should revert this change. It was not made with the knowledge that it would break users. You are the second person to report this, which means there are probably hundreds out there that will be broken by this change. @joshgarnett <https://github.com/joshgarnett> would you mind opening a PR that enables this through an option instead of by default? — Reply to this email directly, view it on GitHub <#4238 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC7SAP243B323AXR6Q4T2L2EFAF5AVCNFSM6AAAAABGTQCFUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRSGI2DKMRZHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>