Transport close after keepalive failure blocks on unresponsive reader

[jgold2-stripe]

What version of gRPC are you using?

v1.71.2

What version of Go are you using (go version)?

go version go1.24.4 darwin/arm64

What operating system (Linux, Windows, …) and version?

MacOS

What did you do?

I have a connection open to a streaming RPC using client keepalives. When the network goes down, the keepalive fires as expected, but the transport still waits much longer than is necessary to finally close.

What did you expect to see?

I assumed that the transport would close almost immediately after the keepalive fails.

What did you see instead?

The transport does close, but not until the underlying network connection fails its current call to Read.

Just as some extra color, I think the issue stems from the following:

• We detect the keepalive failure.
• Before returning with this error, we call Close.
• The close proceeds quickly (I verified with logging) until it blocks on readerDone.
• The readerDone channel is closed when returning from reader(), which itself makes blocking read calls (via the golang.org/x/net/http2 package) on the underlying net.Conn.
• While the actual network is down, TCP does not know this yet, leaving us to wait for a significant amount of time (40s in some cases) to finally close readerDone.
• Once readerDone is closed, the transport completes its close quickly from there.

I noticed that a write deadline is set (though I was surprised to find that the error is ignored -- maybe it should be logged at least?), but not a read deadline. Testing with a local version where I also invoke SetReadDeadline, I can confirm that the transport does close quickly as I'd expect.

Is the absence of a call to SetReadDeadline intentional? I wonder if I'm missing a good reason for it. In our case, since the failure to receive a keepalive ACK necessarily indicates that the connection to the server is dead, we absolutely expect any read to time out, so would prefer not to wait unnecessarily for it. I can propose code changes here, but want to step back and see if I am misunderstanding what should be happening.

[arjan-bal]

Hi @jgold2-stripe, thanks for the detailed bug report. Here's some history:

1. Initially we didn't even have a write deadline. In Send GOAWAY to server on Client Transport Shutdown #7015 we started writing GOAWAY frames when closing the connection. We found that the write may get stuck for a long time, so the deadline was added in transport: add timeout for writing GOAWAY on http2Client.Close() #7371.
2. The channel closing started blocking on the reader goroutine exiting more recently in transport : wait for goroutines to exit before transport closes #7666. Before this change, the channel would close, with the connection still open in the background.

Change 2 seems to have surfaced the reported issue. In my opinion, we should add a read deadline as suggested — we didn’t realize it was needed until cc.Close started blocking on it. Let me confirm this with the other maintainers and get back to you. It may take until next week due to the long weekend in the US.

[arjan-bal]

I discussed this with @dfawley and we think it is safe to add a read deadline in http2Client.Close. I checked all the call sites for Close and verified that they don't care about ongoing RPCs. There is a http2Client.GracefuleClose method that waits for active streams to drain before calling Close. GracefuleClose is used when the transport needs to be closed without failing RPCs.

@jgold2-stripe How long are you proposing the deadline to be? I'm assuming we'd want to keep it the same as the write deadline (i.e., 10s). Also, would you be willing to raise a PR with the fix?

[jgold2-stripe]

I've found that the shorter the better. So 1s works for us, but I don't now well-enough to say whether that would be too aggressive for others. I guess 10s is the most sensible if we want the read and write to stay consistent with each other.

Is it possible to default to 10s but introduce a new DialOption like WithTransportCloseTimeouts(read, write time.Duration) (name and signature are kind of just a placeholder here) and then that DialOption would set new fields in ConnectOptions? Then we could default them to 10s to be consistent with existing code, and callers like us could override?

I am fine to raise the PR once we know what it should contain :)

[dfawley]

For some more background, it seems the mistaken assumption is this:

grpc-go/internal/transport/http2_client.go

Lines 472 to 473 in 12f9d9c

	// server sent a GOAWAY). For I/O errors, the reader will hit it
	// after draining any remaining incoming data.

For I/O errors, the reader will hit it after draining any remaining incoming data.

In that case, we expected we would let the reader drain the data remaining on the connection, hit the same I/O error, then exit. I/O errors are any error returned from conn.Write.

When this was added, I'm pretty sure that was all true and everything would happen as indicated. Later, we added the timeout to the write, without adding one to the read, so we introduced a new kind of "I/O error" that the reader would not see on its own.

Since this GOAWAY is a best-effort thing, and since we're shutting down, I'm OK with shortening the deadline down to 1 second. I don't think something like this needs to be configurable.

[jgold2-stripe]

Thanks -- I will take on submitting the change!

[github-actions]

This issue is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

[jgold2-stripe]

Sorry for not having an update. I am planning to work on this in the next few weeks.