vmagamedov / grpclib

Pure-Python gRPC implementation for asyncio
http://grpclib.readthedocs.io
BSD 3-Clause "New" or "Revised" License
938 stars 91 forks source link

Access underlying HTTP details on GRPCError #170

Open JonatannQm opened 1 year ago

JonatannQm commented 1 year ago

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.

vmagamedov commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Do you think we can make this private API public?

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

xloem commented 1 year ago

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 commented 1 year ago

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