Skip to content
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.

Sign up for GitHub

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

grpc: server should send RST_STREAM when deadline is exceeded #7892

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

grpc: server should send RST_STREAM when deadline is exceeded #7892

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
786724d
Rebase master fork in my local
misvivek Oct 17, 2024
2bf22b3
Merge branch 'grpc:master' into master
misvivek Nov 21, 2024
d49f19f
Merge branch 'grpc:master' into master
misvivek Nov 28, 2024
66674d9
Merge branch 'grpc:master' into master
misvivek Dec 3, 2024
bb6d782
Merge branch 'grpc:master' into master
misvivek Dec 3, 2024
4e19a07
Rebase master fork in my local
misvivek Oct 17, 2024
0cb56a2
rebase the with upstream master branch
misvivek Dec 3, 2024
e59ae3f
Merge branch 'grpc:master' into master
misvivek Dec 3, 2024
a75629c
Revert "Merge branch 'grpc:master' into master"
misvivek Dec 3, 2024
6e6b498
pr ref
misvivek Dec 3, 2024
fb004c2
revert the file rpc_util_test clientcon_state__trasition
misvivek Dec 4, 2024
0cf3584
added the override function in hadle server
misvivek Dec 4, 2024
dea8408
added select statement for case
misvivek Dec 4, 2024
86a4997
verify the data in the dat available
misvivek Dec 5, 2024
5aada36
change done on checkHeaderandtrailer
misvivek Dec 9, 2024
961f5e5
add the comment
misvivek Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions internal/transport/handler_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ func handleStreamCloseBodyTest(t *testing.T, statusCode codes.Code, msg string)
st.ht.HandleStreams(
context.Background(), func(s *ServerStream) { go handleStream(s) },
)

wantHeader := http.Header{
"Date": nil,
"Content-Type": {"application/grpc"},
Expand Down Expand Up @@ -392,7 +393,23 @@ func (s) TestHandlerTransport_HandleStreams_Timeout(t *testing.T) {
t.Errorf("ctx.Err = %v; want %v", err, context.DeadlineExceeded)
return
}
// rst flag setting to verify the noop function: signalDeadlineExceeded
ch := make(chan struct{}, 1)
origSignalDeadlineExceeded := signalDeadlineExceeded
signalDeadlineExceeded = func() {
ch <- struct{}{}
}
defer func() {
signalDeadlineExceeded = origSignalDeadlineExceeded
}()
s.WriteStatus(status.New(codes.DeadlineExceeded, "too slow"))
select {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have to wait on channel as well otherwise you are not verifying the if your function was triggered

select {
case <-ch:  // Signal received, continue with the test
case <-s.ctx.Done():
case <-time.After(5 * time.Second):
	t.Errorf("timeout waiting for ctx.Done")
	return
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@misvivek i just saw you are doing this inside goroutine which won't affect the execution of test whether pass or fail.

You need to move both no op function setting and channel waiting code outside of go routine and check at the end when go runStream(s) is done, perhaps at the end after checking the trailers. So I would structure it like this

// rst flag setting to verify the noop function: signalDeadlineExceeded
ch := make(chan struct{}, 1)
origSignalDeadlineExceeded := signalDeadlineExceeded
signalDeadlineExceeded = func() {
ch <- struct{}{}
}
defer func() {
signalDeadlineExceeded = origSignalDeadlineExceeded
}()

runStream := func(s *ServerStream) {....}

...
...

checkHeaderAndTrailer(t, rw, wantHeader, wantTrailer)
select {
case <-ch: // Signal received, continue with the test
case <-time.After(5 * time.Second):
t.Errorf("timeout waiting for ctx.Done")
return
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And i think the test will fail then because you haven't yet fixed the bug.

case <-ch: // Signal received, continue with the test
case <-s.ctx.Done():
case <-time.After(5 * time.Second):
t.Errorf("timeout waiting for ctx.Done")
return
}
}
ht.HandleStreams(
context.Background(), func(s *ServerStream) { go runStream(s) },
Expand Down
5 changes: 5 additions & 0 deletions internal/transport/http2_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,6 +1100,9 @@ func (t *http2Server) writeStatus(s *ServerStream, st *status.Status) error {
}
// Send a RST_STREAM after the trailers if the client has not already half-closed.
rst := s.getState() == streamActive
if rst {
signalDeadlineExceeded()
}
t.finishStream(s, rst, http2.ErrCodeNo, trailingHeader, true)
for _, sh := range t.stats {
// Note: The trailer fields are compressed with hpack after this call returns.
Expand All @@ -1111,6 +1114,8 @@ func (t *http2Server) writeStatus(s *ServerStream, st *status.Status) error {
return nil
}

var signalDeadlineExceeded = func() {}

// Write converts the data into HTTP2 data frame and sends it out. Non-nil error
// is returns if it fails (e.g., framing error, transport error).
func (t *http2Server) write(s *ServerStream, hdr []byte, data mem.BufferSlice, _ *WriteOptions) error {
Expand Down
Loading