grpc: add ConnectionInfo to client's ResponseHeaders and Trailers

[dfawley]

Also: return ConnectionInfo from Transport::connect instead of SecurityInfo (yes, I know I just changed it to produce SecurityInfo) so that the subchannel can set the full ConnectionInfo in Trailers when producing errors locally, since the address of the subchannel may be relevant to those errors.

FWIW I wanted to make the subchannel apply an interceptor that could always set ConnectionInfo in response streams (either ResponseHeaders or in Trailers-only responses), but because the field is mandatory in ResponseHeaders, the transport would need to set it to some default/empty value, which felt wrong. Anything producing a ResponseHeaders or Trailers should ensure the ConnectionInfo is set correctly.

cc @joshuatants as FYI - this is the change that will provide you with the ability to determine the server's address.

[dfawley]

@arjan-bal - I had to merge in the cancellation changes that you recently submitted. I'm a little uncertain about the tonic behavior if we call cancel on the token after the stream has ended naturally -- i.e. the tonic recv stream returns Ok(None) or Err(status).

In these cases, IIUC tonic itself should send a RST_STREAM(NO_ERROR) as soon as the server ends the stream (via trailers or RST_STREAM) if the client was not already half-closed, or do nothing if it was, and ignore the cancellation token. Do you know if tonic is already doing all this correctly?

[ejona86]

Didn't have time to look at it deeply.

[arjan-bal]

@arjan-bal - I had to merge in the cancellation changes that you recently submitted. I'm a little uncertain about the tonic behavior if we call cancel on the token after the stream has ended naturally -- i.e. the tonic recv stream returns Ok(None) or Err(status).

In these cases, IIUC tonic itself should send a RST_STREAM(NO_ERROR) as soon as the server ends the stream (via trailers or RST_STREAM) if the client was not already half-closed, or do nothing if it was, and ignore the cancellation token. Do you know if tonic is already doing all this correctly?

Tonic doesn't seem to couple the request and response streams. When the response stream ends, the request stream is left uninterrupted. I verified this by writing an H2 server that immediately sends response headers, a message, and trailers (with EOS) without sending a RST_STREAM. In this scenario, I observed Tonic's recv stream end with None, but the request stream kept sending messages, which were successfully received by the server.

At the gRPC layer, cancellation is triggered when the RecvStream is dropped, rather than when trailers are received. Additionally, the error code used for the reset frame is CANCEL.

We could modify the cancel method to accept an error (maybe tonic::Status?) that controls the RST code, allowing the gRPC Tonic transport to send a cancellation with NO_ERROR once trailers are received. However, making this change within Tonic itself might result in a breaking behavior change. Note that the underlying h2 layer already ensures that RST_STREAM frames are ignored if the stream is already fully closed.

[dfawley]

making this change within Tonic itself might result in a breaking behavior change

Maybe. But the behavior I'm describing is a standard gRPC-over-h2 behavior. The library on both sides should send a RST_STREAM when the server ends the stream and the client hasn't half-closed already. There is no reason to allow the client to keep sending requests, as the server will discard them (or should, anyway - it's unclear from your tests whether a tonic server would keep delivering request messages even if an h2 server would see them).

We could modify the cancel method to accept an error (maybe tonic::Status?) that controls the RST code

I don't think we need/want anything this complicated. The application doesn't need to control this. The library should ideally do the right thing automatically.

[arjan-bal]

Some private fields and a comment still use "peer" instead of "connection". Otherwise, LGTM.

[arjan-bal]

nit: The comment still mentions PeerInfo.

[dfawley]

Fixed

[dfawley]

Fixed all the lingering "peer" references.

[dfawley]

Fixed

[arjan-bal]

LGTM