search

fullstorydev / grpchan

Channels for gRPC: custom transports
MIT License
204 stars 23 forks source link

Propate http request errors to the write side of a stream #59

Closed dragonsinth closed 3 years ago

dragonsinth commented 3 years ago

Before:

> fsgrpcurl -max-time=3s cog list
Failed to list services: io: read/write on closed pipe

After:

> fsgrpcurl -max-time=3s cog list
failed to create a grpc channel: context deadline exceeded: connection error: desc = "transport: Error while dialing dial tcp 127.0.0.1:XXXX: connect: connection refused"
jhump commented 3 years ago 
> fsgrpcurl -max-time=3s cog list
failed to create a grpc channel: context deadline exceeded: connection error: desc = "transport: Error while dialing dial tcp 127.0.0.1:XXXX: connect: connection refused"

🤮 I guess that tells us that WithReturnConnectionError() doesn't actually work. This is a dial issue, so the dial call should have returned this error. Apparently, it returned no error and deferred the problem to the first RPC 😭

dragonsinth commented 3 years ago 
> fsgrpcurl -max-time=3s cog list
failed to create a grpc channel: context deadline exceeded: connection error: desc = "transport: Error while dialing dial tcp 127.0.0.1:XXXX: connect: connection refused"

🤮 I guess that tells us that WithReturnConnectionError() doesn't actually work. This is a dial issue, so the dial call should have returned this error. Apparently, it returned no error and deferred the problem to the first RPC 😭

This is httpgrpc... no real dial actually happens until you attempt to make a request, right? Because it's an opaque http transport under the hood.

jhump commented 3 years ago

This is httpgrpc... no real dial actually happens until you attempt to make a request, right? Because it's an opaque http transport under the hood.

🤦 doh, my bad. In that case, this is probably as good as we can do!

dragonsinth commented 3 years ago

I mean, if we wanted tighter integration, perhaps the httpgrpc chan could issue a test http query during the "connect" phase, just to get the Transport to make an initial conn. Maybe like, a HEAD request to http://host:port/ ?

jhump commented 3 years ago

That seems overly cumbersome. I think it's fine as you just merged it.

dragonsinth commented 3 years ago

👍 yeah just thinking out loud :D