-
Notifications
You must be signed in to change notification settings - Fork 4.6k
transport: Invoke net.Conn.SetWriteDeadline
in http2_client.Close
#8534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
be96fc2
2bd76dc
4e305d0
2645cc8
75a8d96
b9e53e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3008,6 +3008,60 @@ func (s) TestClientCloseReturnsEarlyWhenGoAwayWriteHangs(t *testing.T) { | |
ct.Close(errors.New("manually closed by client")) | ||
} | ||
|
||
// deadlineTestConn is a net.Conn wrapper used to assert that deadlines are set | ||
// during http2Client.Close(). | ||
type deadlineTestConn struct { | ||
net.Conn | ||
// We use atomic.Bool here since there may be more than one call to | ||
// http2Client.Close -- which sets these deadlines -- and not all of them | ||
// from the same goroutine as our test. In fact we only care about the first | ||
// such invocation, which *does* come from the main goroutine of our test, | ||
// but the race detector can't know that and complains (understandably) | ||
// about writes from those successive calls when these variables are not | ||
// atomic.Bool. | ||
// | ||
// For more detailed background, see | ||
// https://github.com/grpc/grpc-go/pull/8534#discussion_r2297717445 . | ||
observedReadDeadline atomic.Bool | ||
observedWriteDeadline atomic.Bool | ||
arjan-bal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
func (c *deadlineTestConn) SetReadDeadline(t time.Time) error { | ||
c.observedReadDeadline.Store(true) | ||
return c.Conn.SetReadDeadline(t) | ||
} | ||
|
||
func (c *deadlineTestConn) SetWriteDeadline(t time.Time) error { | ||
c.observedWriteDeadline.Store(true) | ||
return c.Conn.SetWriteDeadline(t) | ||
} | ||
|
||
// Tests that connection read and write deadlines are set as expected during | ||
// Close(). | ||
func (s) TestCloseSetsConnectionDeadlines(t *testing.T) { | ||
dialer := func(_ context.Context, addr string) (net.Conn, error) { | ||
conn, err := net.Dial("tcp", addr) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return &deadlineTestConn{Conn: conn}, nil | ||
} | ||
co := ConnectOptions{ | ||
Dialer: dialer, | ||
} | ||
server, client, cancel := setUpWithOptions(t, 0, &ServerConfig{}, normal, co) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we were to set read/write deadlines during handshaking (we currently only block on the context here, but our server does set deadlines here), then this test would be invalid. I think we should either confirm that the values are still |
||
defer cancel() | ||
defer server.stop() | ||
client.Close(fmt.Errorf("closed manually by test")) | ||
dConn := client.conn.(*deadlineTestConn) | ||
if !dConn.observedReadDeadline.Load() { | ||
t.Errorf("Connection read deadline was never set") | ||
} | ||
if !dConn.observedWriteDeadline.Load() { | ||
t.Errorf("Connection write deadline was never set") | ||
} | ||
} | ||
|
||
// TestReadHeaderMultipleBuffers tests the stream when the gRPC headers are | ||
// split across multiple buffers. It verifies that the reporting of the | ||
// number of bytes read for flow control is correct. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better/simpler to just use
SetDeadline
with the same 1 second deadline, if possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible to do that, but am a little concerned about throwing that in here with the other changes, only because it seems like a possible load-bearing change for which I can't really vouch myself. I took a look back at #7371, where the write deadline was added, and couldn't seem to find any direct discussion about why the value of 10 was chosen, or whether a value of 1 might be problematic.
I don't mind just flipping this to 1, but also wonder if you prefer that we not do that in this PR, and then change it later? That way if everything else is good but the change from 10 to 1 introduces some separate problem, it's a simple revert of just that one-liner PR?
Not sure how your team likes to balance risk:throughput.