public / sonora

A gRPC-Web implementation for Python
Apache License 2.0
244 stars 13 forks source link

Missing trailers frame in unary response when no trailers metadata is specified #47

Closed parabolala closed 2 years ago

parabolala commented 2 years ago

Current code has:

       if context._trailing_metadata:
            trailers = context._trailing_metadata

            trailer_message = protocol.pack_trailers(trailers)
            trailer_data = wrap_message(True, False, trailer_message)
        else:
            trailer_data = b""
...
        await send(
            {"type": "http.response.body", "body": message_data, "more_body": True}
        )
        await send(
            {"type": "http.response.body", "body": trailer_data, "more_body": False}
        )

This results in no trailer_data frame (i.e. nothing) being sent if no context._trailing_metadata is specified instead of non-empty frame containing no data. This seems to violate the gRPC-Web expectations in at least some clients (and maybe in the spec?) that the trailers frame should always be present even if it's data is missing. One particular client unhappy with missing trailers is protobuf-ts (fails with trailers missing). grpc-dart mentions a similar error.

I was able to workaround the issue by changeing the above code to:

         if context._trailing_metadata:
            trailers = context._trailing_metadata
        else:
            trailers = b""

        trailer_message = protocol.pack_trailers(trailers)
        trailer_data = wrap_message(True, False, trailer_message)
public commented 2 years ago

Thanks for this bug report. I'll push a fix. I think I am actually dealing with Status incorrectly in general right now as I put it in Headers rather than Trailers for unary calls.

I think a strict reading of the spec would require grpc-status to only be present in trailers unless it was a trailers-only response, in which case all trailers would actually just be sent as headers.

public commented 2 years ago

There's an open draft PR for this. Any chance you could check it out and see if it fixes your problem?

I need to re-do the interop tests again because Google keep breaking them :(

parabolala commented 2 years ago

I added the repro code at https://github.com/parabolala/sonora-47-repro