Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access underlying HTTP details on GRPCError #170

Open
JonatannQm opened this issue Feb 22, 2023 · 6 comments · May be fixed by #171
Open

Access underlying HTTP details on GRPCError #170

JonatannQm opened this issue Feb 22, 2023 · 6 comments · May be fixed by #171

Comments

@JonatannQm
Copy link

Add HTTP details to the GRPCError exception for handling various errors and flows related to HTTP.

For example, the 30X responses (moved) cannot be handled without access to the HTTP headers to fetch the new location.

@JonatannQm JonatannQm linked a pull request Feb 22, 2023 that will close this issue
@vmagamedov
Copy link
Owner

gRPC has (almost) nothing to do with HTTP semantics, it is abstracted away, so you shouldn't know about HTTP/2 or any other protocol underneath.

When you're making a gRPC request you should know in advance that endpoint on the server supports HTTP/2 protocol and can pass/handle gRPC requests. There is even no protocol negotiation/upgrade between client and server.

For reference you can check out grpcio API. They also don't share low-level details about HTTP errors.

If in case of unexpected HTTP errors grpclib shows not informative enough error messages then I'm ok to accept PR with better error messages. Currently grpclib treats all unexpected errors (not specified in gRPC protocol spec) as unrecoverable.

If your example is not imaginary and you have to deal with unknown servers, then you may use any HTTP/2-capable client to make first request and detect 30X responses. But I really can't imagine such situation.

@JonatannQm
Copy link
Author

My suggestion is not to change the behavior of grpclib regarding unexcepted errors. I understand that gRPC is and should not handle these errors.

In my opinion, adding underlying HTTP information (if available) to the error does not break this statement.

I am only suggesting including the information in the error; as you can see in the PR that I opened (#171), the headers already exist in most of the functions that create the error. It's only a matter of passing it forward.

The example I gave is not imaginary. It's a problem that I have, which is why I suggested this change.

@vmagamedov
Copy link
Owner

It is currently possible to get headers using private API:

stream = None
try:
    async with greeter.SayHello.open() as stream:
        await stream.send_message(HelloRequest(name='Dr. Strange'), end=True)
        reply = await stream.recv_message()
        print(reply.message)
except GRPCError:
    if stream:
        print(stream._stream.headers)
    raise

Sorry but currently I don't want to introduce new APIs. Only if many users will benefit from them.

@JonatannQm
Copy link
Author

Do you think we can make this private API public?

If it's private, API should not be used outside the class.

@xloem
Copy link
Contributor

xloem commented Jun 28, 2023

The full error details on e.g. cloudflare errors are generally in the page body rather than the headers; how does one access the body of the failing response?

@vmagamedov
Copy link
Owner

grpclib only handles gRPC errors, which are defined here: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#errors

It doesn't handles HTTP errors like generic http clients. For grpclib they are all the same – unrecoverable transport errors.

Here is quick and dirty way to receive body:

    async with Channel('www.google.com', 443, ssl=True) as channel:
        greeter = GreeterStub(channel)
        async with greeter.SayHello.open() as stream:
            await stream.send_message(HelloRequest(name='Dr. Strange'), end=True)
            try:
                reply = await stream.recv_message()
                print(reply.message)
            except GRPCError:
                print(stream._stream.headers)
                size = int(dict(stream._stream.headers)['content-length'])
                print(await stream._stream.recv_data(size))
                raise  # this is critical to reraise original exceptions inside `.open()` context-manager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants