Incorrect client disconnection error codes

[emcfarlane]

Describe the bug

On client disconnections for both HTTP1 and HTTP2 transports we should detect the client has disconnected and use an appropriate error code. Currently disconnects can see a connect.CodeUknown causing erroneous error reporting. To detect a client disconnect we could check the context for cancellation (see docs from deprecated http.CloseNotifier).

To Reproduce

Create a connection and close the underlying net.Conn to replicate a dropped client connection:

var conn client.Conn // Captured client connection.
transport := server.Transport()
dialContext := transport.DialTLSContext
transport.DialTLSContext = func(ctx context.Context, network, addr string, cfg *tls.Config) (net.Conn, error) {
  conn, _ = dialContext(ctx, network, addr, cfg) // Capture the client connection.
  return conn, nil
}
stream := ... // Create stream
stream.Send(nil) // Send headers
conn.Close() // Abruptly close
// Check handler error codes

Handler errors will be (only checked the envelope flow):

• http1/read: invalid_argument: protocol error: incomplete envelope: unexpected EOF
• http1/write: unknown: write envelope: io: read/write on closed pipe
• http2/read: invalid_argument: protocol error: incomplete envelope: client disconnected
• http2/write: unknown: write envelope: client disconnected

In all cases checking the context error will return a context.Cancelled error. The http2 client disconnect string is from this error.

Environment (please complete the following information):

• connect-go version or commit: v0.1.12
• go version: go version go1.18.3 darwin/amd64

Additional context

Thanks to the reports from:

• @pqn https://bufbuild.slack.com/archives/CRZ680FUH/p1701370334963849
• @njiang747 https://bufbuild.slack.com/archives/CRZ680FUH/p1700005011055009

[emcfarlane]

Related issue: golang/go#52183 . Which seems to showclient disconnects can be reported without a context cancellation.

[jhump]

FWIW, closing the actual connection is not the same as a cancellation with http/2. With http/2, the client aborts the stream, but the connection remains open. With http/1.1, since multiplexing a single connection is not supported, the only way a client can cancel is to disconnect.

Though perhaps the server handler doesn't care about distinguishing the two cases -- in other words, maybe it doesn't care to distinguish the client intentionally cancelling a single stream vs. a network partition implicitly cancelling all streams (without the client's intent).

[pqn]

Hi, just wanted to check in on the prioritization of this, particularly for the http2 case.

[jhump]

@pqn, looks like @emcfarlane has opened a pull request, so hopefully it can be addressed in the next release.

[jhump]

This should be addressed in v1.15.0 (resolved in #659).