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

Delay closing connection on the server after all streams are completed #6957

Closed
wants to merge 4 commits into from
Closed

Delay closing connection on the server after all streams are completed #6957

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8f0acbe
Delay closing connection on the server after all streams are completed
s-matyukevich Feb 1, 2024
ede0afe
increase sleep interval
s-matyukevich Feb 1, 2024
f61ec45
set timeout to 0 in tests
s-matyukevich Feb 1, 2024
cac6dc2
Add comment
s-matyukevich Feb 1, 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
12 changes: 12 additions & 0 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"strings"
"sync"
"sync/atomic"
"testing"
"time"

"golang.org/x/net/trace"
Expand Down Expand Up @@ -66,6 +67,8 @@
listenerAddressForServeHTTP = "listenerAddressForServeHTTP"
)

var goAwayTimeout = 1 * time.Second

func init() {
internal.GetServerCredentials = func(srv *Server) credentials.TransportCredentials {
return srv.opts.creds
Expand All @@ -83,6 +86,10 @@
internal.BinaryLogger = binaryLogger
internal.JoinServerOptions = newJoinServerOption
internal.RecvBufferPool = recvBufferPool

if testing.Testing() {

Check failure on line 90 in server.go

View workflow job for this annotation

GitHub Actions / tests (tests, 1.20) 

undefined: testing.Testing

Check failure on line 90 in server.go

View workflow job for this annotation

GitHub Actions / tests (tests, 1.20) 

undefined: testing.Testing

Check failure on line 90 in server.go

View workflow job for this annotation

GitHub Actions / tests (tests, 1.20) 

undefined: testing.Testing

Check failure on line 90 in server.go

View workflow job for this annotation

GitHub Actions / tests (tests, 1.20) 

undefined: testing.Testing

Check failure on line 90 in server.go

View workflow job for this annotation

GitHub Actions / tests (tests, 1.19) 

undefined: testing.Testing

Check failure on line 90 in server.go

View workflow job for this annotation

GitHub Actions / tests (tests, 1.19) 

undefined: testing.Testing

Check failure on line 90 in server.go

View workflow job for this annotation

GitHub Actions / tests (tests, 1.19) 

undefined: testing.Testing

Check failure on line 90 in server.go

View workflow job for this annotation

GitHub Actions / tests (tests, 1.19) 

undefined: testing.Testing
Copy link
Member

Choose a reason for hiding this comment

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

seems like testing.Testing() was only made available since go.1.21.0 https://pkg.go.dev/testing#Testing

We maintain a backward compatibility to the last 3 versions of go. (i.e., v1.19+)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions what should I do about it? Options I can think of:

  • add a hack to determine if this is a testing build, something like this https://stackoverflow.com/questions/14249217/how-do-i-know-im-running-within-go-test
  • Don't enable the delay by default, add a server option to activate it
  • Enable delay by default and ignore the fact that it could break some tests that expect the server to be shut-down faster then 1 sec. Add a server option so that users can disable (or maybe configure?) the delay in their tests.

goAwayTimeout = 0
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests that use grpc.NewServer and define very short timeouts should not be affected.

}

var statusOK = status.New(codes.OK, "")
Expand Down Expand Up @@ -1013,6 +1020,11 @@
}

defer func() {
// The client might not be done reading all the data, which at this point
// is stored in the kernel TCP buffer, yet. If we close the connection right away
// the client will get RST and the request will fail. Delay closing for 1 sec.
// See more details in https://github.com/grpc/grpc-go/pull/6957
time.Sleep(goAwayTimeout)
st.Close(errors.New("finished serving streams for the server transport"))
for _, sh := range s.opts.statsHandlers {
sh.HandleConn(ctx, &stats.ConnEnd{})
Expand Down
Loading