handleForwardResponseOptions not called by DefaultHTTPError

[movsb]

handleForwardResponseOptions is called by both ForwardResponseStream and ForwardResponseMessage but not DefaultHTTPError. Is there a reason why? Is it intended?

FYI: handleForwardResponseServerMetadata, handleForwardResponseTrailerHeader and handleForwardResponseTrailer are called by ForwardResponseMessage and DefaultHTTPError.

Steps you follow to reproduce the error:

1. Register a forwardResponseOption:

mux := runtime.NewServeMux(runtime.WithForwardResponseOption(myCallback))

2. Return an error from GRPC handler.

What did you expect to happen instead:

myCallback gets called on all response-related functions like ForwardResponseMessage, ForwardResponseStream and DefaultHTTPError.

What's your theory on why it isn't working:

[johanbrandhorst]

Hm, yea this might be a reasonable proposal, but I don't think we can really change this without breaking users. Might have to be another one for the v2 dust heap 😬.

[movsb]

If I'm the first one who filed this issue, maybe I can guess they don't use this feature yet? 😂 Otherwise, they must encounter this. @johanbrandhorst

[johanbrandhorst]

Nah, this feature is definitely used I'm afraid. I use it myself for many applications and never actually wanted this behaviour. I'm afraid you'll have to overwrite DefaultHTTPError with a function that does what you want :(.

[movsb]

Yeah, I'm trying. But... DefaultHTTPError uses some unexported global functions and unexported fields of ServeMux. My own error function may be hard to write if we want to cover all these situations what DefaultHTTPError've done already.

Maybe we can provide the other runtime.WithForwardResponseOption function that sets a flag to indicate that the option would also be called by DefaultHTTPError, which defaults to false. So it matches the current behavior.

[johanbrandhorst]

I'm hesitant to add more knobs when there is a work around and not a great deal of demand. Does it not work to just assign runtime.HTTPError to a function that calls your function and wraps runtime.DefaultHTTPError?

[movsb]

Thanks! It does work.

runtime.HTTPError = func(ctx context.Context, mux *runtime.ServeMux, marshaler runtime.Marshaler, w http.ResponseWriter, req *http.Request, err error) {
    for _, opt := range mux.GetForwardResponseOptions() {
        // The response body (message) is always nil here
        if err := opt(ctx, w, nil); err != nil {
            return
        }
    }
    runtime.DefaultHTTPError(ctx, mux, marshaler, w, req, err)
}