feat: skip calling http Write method when response is of empty type

[reversearrow]

References to other Issues or PRs

#4901
#240

Have you read the Contributing Guidelines?

Yes

Brief description of what is fixed or changed

Skip calling the Write() method on the HTTP response writer when the response is empty. This is to avoid code from keep printing the error message below.

2024/11/04 10:49:00 ERROR: Failed to write response: http: request method or response status code does not allow body

Other comments

[reversearrow]

@johanbrandhorst can you please review this PR? Thank you

[johanbrandhorst]

Could you please add a test for this behavior? It might be possible to capture the log output in an integration test (https://github.com/grpc-ecosystem/grpc-gateway/blob/main/examples/internal/integration/integration_test.go).

[johanbrandhorst]

I understand the problem, but I don't think this is the right conditional. The error is because we're trying to write a body when the request method doesn't allow it. Shouldn't we just check if req.Method == http.MethodGet or something like that?

[reversearrow]

Thanks @johanbrandhorst for the review.

The error occurs because status codes 204 and 304 do not allow a response body.

Ideally, we should check if the response status is one of these codes before attempting to write a body. However, there isn’t a straightforward way to retrieve the status code from http.ResponseWriter after it has been written.

Also, req.Method == http.MethodGet alone is not sufficient, as a GET request can still return a non-empty response body. We could add a check for DELETE method calls, but again, there’s no guarantee that they will always have an empty response.

[johanbrandhorst]

OK, alternative idea. How about we intercept the error and just don't log it?

Suggested change

	if _, ok := resp.(*emptypb.Empty); !ok {
	if _, err = w.Write(buf); err != nil {
	grpclog.Errorf("Failed to write response: %v", err)
	}
	if _, err = w.Write(buf); err != nil && !errors.Is(err, http.ErrBodyNotAllowed) {
	grpclog.Errorf("Failed to write response: %v", err)
	}

[reversearrow]

Works for me, thank you. Addressed it here ff571ca

[johanbrandhorst]

OK, alternative idea. How about we intercept the error and just don't log it?

Suggested change

	if _, ok := resp.(*emptypb.Empty); !ok {
	if _, err = w.Write(buf); err != nil {
	grpclog.Errorf("Failed to write response: %v", err)
	}
	if _, err = w.Write(buf); err != nil && !errors.Is(err, http.ErrBodyNotAllowed) {
	grpclog.Errorf("Failed to write response: %v", err)
	}