Client Stream Recv not interrupted when closing connection after GracefulStop

[bernerdschaefer]

In one of our test suites, we intermittently see test hangups when using client streams and GracefulStop on our test server.

I've put together an example program which demonstrates this: https://github.com/bernerdschaefer/grpc-stream-close-graceful-stop

It includes test cases for the order of operations between closing a client connection and calling GracefulStop on the server. When conn.Close is called after GracefulStop, the stream's Recv call does not get interrupted.

I would expect the order of operations not to matter here, particularly if this could be triggered for non-local gRPC connections.

For now, we work around this by introducing a context which we cancel on Close as demonstrated in this commit: bernerdschaefer/grpc-stream-close-graceful-stop@e257f41.

If it's expected that streams be explicitly canceled separate from the connection, I'd expect that to be highlighted in the docs or examples.

[MakMukhi]

Thanks for the reproduction repository, made it really easy to pin-point the issue.
We have identified the code path that leads to this behavior and are trying to reason about what's the right thing to do:

1. Close all streams in flight when ClientConn is closed.
2. Have the users explicitly close all streams despite having called ClientConn.Close().

As a user do you have a preference or a use case that favors either of the behaviors?

[bernerdschaefer]

My initial expectation of ClientStream was that it would have a Close() error method, with usage like:

conn, _ := grpc.Dial()
defer conn.Close()

stream, _ := NewClient(conn).StreamUpdates(...)
defer stream.Close()

The absence of such a Close method on the stream lead me to assume that closing the ClientConn would close open streams.

I don't know that I have a strong preference either way, except to say that it's not obvious from the current functions and interfaces that you must manage cancelation of the stream context yourself.

I can see how the second option might be a big "gotcha" if the interface can't be updated to include Close, as any documentation around that wouldn't end up in user's own generated client packages.

[MakMukhi]

Thanks again for filing the issue with a repro.