Fetch status from either headers or trailers

[dimitris-m]

Closes dialohq/ocaml-grpc#1 for Lwt & Async.

In summary, to deal with "trailers-only" responses, we apply trailers_handler also to the headers. This seems to successfully solve the situation in which some calls hang, as reported in #2.

Although we should reasonably assume that a response will not contain grpc-status in both headers and trailers, in the unlikely event that a server has this behaviour, no exception will be raised. However, only one status will be recorded.

In the event that there is no status at all, the call can still hang. However, this is unlikely.

[quernd]

Brilliant, thank you! @mbacarella can you confirm this works for your case?

In the event that there is no status at all, the call can still hang. However, this is unlikely.

I wonder if we should provision for that. It's unlikely to happen with a well-behaved gRPC server, but we should probably raise an exception. Hanging silently doesn't seem like the right thing to do when we have consumed the entire body and are not waiting for anything anymore. What do you think?

[dimitris-m]

I wonder if we should provision for that. It's unlikely to happen with a well-behaved gRPC server, but we should probably raise an exception. Hanging silently doesn't seem like the right thing to do when we have consumed the entire body and are not waiting for anything anymore. What do you think?

Thank you for raising the issue!

About the case of no grpc-status, I restored the check by @mbacarella, adapting it also to the Lwt client. If that happens, we now return status Unknown but I also added a custom message "Server did not return grpc-status".

I suggest we don't raise, given that in https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses it says:
Implementations should expect broken deployments to send non-200 HTTP status codes in responses as well as a variety of non-GRPC content-types and to omit Status & Status-Message. Implementations must synthesize a Status & Status-Message to propagate to the application layer when this occurs.

[wokalski]

@doctor-pi do you use discord? We have a discord group for contributors. Feel free to message me on discord @wokalski to add you to it. The group is not too active/spammy but it might be useful from time to time!

After this pull request gets merged, I will push it to dialohq/ocaml-grpc and archive this repo.

[quernd]

I suggest we don't raise, given that in https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses it says: Implementations should expect broken deployments to send non-200 HTTP status codes in responses as well as a variety of non-GRPC content-types and to omit Status & Status-Message. Implementations must synthesize a Status & Status-Message to propagate to the application layer when this occurs.

Good call. I hadn't noticed this scenario was actually part of the specification.