search

dialohq / ocaml-grpc

gRPC library for OCaml
https://dialohq.github.io/ocaml-grpc
BSD 3-Clause "New" or "Revised" License
58 stars 9 forks source link

Report HTTP2 error status when it's not OK #22

Closed quernd closed 11 months ago

quernd commented 1 year ago

Arguably, we should report the HTTP2 error status instead of a fake gRPC status when the client receives a non-200 response, e.g. in this situation (quote from the specs):

If Content-Type does not begin with "application/grpc", gRPC servers SHOULD respond with HTTP status of 415 (Unsupported Media Type)

tmcgilchrist commented 1 year ago

This would be useful, I noticed this error when testing against other grpc implementations.

quernd commented 1 year ago

This would be useful, I noticed this error when testing against other grpc implementations.

Great! I need to clean up some conflicts after merging #26 though.

tmcgilchrist commented 1 year ago

Looks like some of the example code is broken with this change. It might be easier to merge https://github.com/dialohq/ocaml-grpc/pull/25 since it fixes a number of packaging issues and then re-do the minimal fixes here.

quernd commented 1 year ago

Looks like some of the example code is broken with this change. It might be easier to merge #25 since it fixes a number of packaging issues and then re-do the minimal fixes here.

Well spotted. I merged #25 and applied the minimal fixes just to get green light from CI. Would you mind taking a look? Maybe you'd want to handle errors differently in the routeguide tutorial (currently the gRPC status is ignored).

quernd commented 1 year ago

Also I updated the docs since this would be a breaking API change.

quernd commented 1 year ago

I had to revert a commit because it caused the client to hang on the second request over a connection. What happens is that we won't get past this line:

  let* response = response in

This is strange because the response promise should be fulfilled by response_handler, not the gRPC handler. I still think we shouldn't call the gRPC handler unless the HTTP status is 200 but I need to look into why this happens.

quernd commented 1 year ago

The hanging (at least with our greeter tests) can be avoided with ~flush_headers_immediately:true. My understanding of what happens is this: If we don't flush the headers immediately (in order to coalesce them with the first frame of the body), the server will not respond immediately either. Waiting for the HTTP status to proceed then puts us into a deadlock because we won't send anything (and thus flush the headers) before we have the response status, but we won't get the response headers before we send our own headers.

I don't know how it impacts performance to flush the headers or not. I think it's definitely necessary to flush them in some cases though, e.g. bidirectional streaming. The client doesn't necessarily have to send any messages in order to receive streaming messages from the server.

On the other hand, maybe we can't rely on the server sending the response headers before we have sent the entire body, so maybe we should always immediately invoke the gRPC handler anyway?

quernd commented 1 year ago

@tmcgilchrist since you mentioned you noticed this error as well, would you mind taking a look? The bulk of the changes are in the docs, the changes in the code are very few (including a small refactor).

quernd commented 11 months ago

Thanks @tmcgilchrist

Docs generatation is just make docs, there's an issue about automating this on merge to main but that hasn't been done yet.

tmcgilchrist commented 11 months ago

Docs updated thanks @quernd. I'll merge this once CI is happy with it.