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.

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

Conversation

misvivek
Copy link

@misvivek misvivek commented Dec 3, 2024

Fixes: #2886

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.95%. Comparing base (634497b) to head (86a4997).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7892      +/-   ##
==========================================
- Coverage   82.04%   81.95%   -0.09%     
==========================================
  Files         377      377              
  Lines       38117    38183      +66     
==========================================
+ Hits        31272    31294      +22     
- Misses       5548     5585      +37     
- Partials     1297     1304       +7     
Files with missing lines Coverage Δ
internal/transport/http2_server.go 90.99% <100.00%> (+0.30%) ⬆️

... and 22 files with indirect coverage changes

@purnesh42H purnesh42H added this to the 1.70 Release milestone Dec 5, 2024
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.

@misvivek misvivek requested a review from purnesh42H December 5, 2024 13:26
@purnesh42H
Copy link
Contributor

purnesh42H commented Dec 6, 2024

@misvivek you need to write the test as I mentioned above and then verify if the test is failing. If it is, then we have to fix the bug first.

@purnesh42H purnesh42H changed the title grpc-go transport: Implement rst stream sending on deadline exceeded grpc: server should send RST_STREAM when deadline is exceeded Dec 6, 2024
Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

Need to fix the bug as well along with test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server should send RST_STREAM when deadline is exceeded
2 participants