-
Notifications
You must be signed in to change notification settings - Fork 4.6k
grpc: deprecate InitialWindowSize and introduce InitialStreamWindowSize #8789
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
c6abd37
b3a849a
abddfcf
3f4f685
0ff0c44
e9acd82
e91fa27
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 |
|---|---|---|
|
|
@@ -490,6 +490,7 @@ type test struct { | |
| unaryServerInt grpc.UnaryServerInterceptor | ||
| streamServerInt grpc.StreamServerInterceptor | ||
| serverInitialWindowSize int32 | ||
| isServerStaticWindow bool | ||
| serverInitialConnWindowSize int32 | ||
| customServerOptions []grpc.ServerOption | ||
|
|
||
|
|
@@ -505,14 +506,16 @@ type test struct { | |
| // Used to test the new compressor registration API UseCompressor. | ||
| clientUseCompression bool | ||
| // clientNopCompression is set to create a compressor whose type is not supported. | ||
| clientNopCompression bool | ||
| unaryClientInt grpc.UnaryClientInterceptor | ||
| streamClientInt grpc.StreamClientInterceptor | ||
| clientInitialWindowSize int32 | ||
| clientInitialConnWindowSize int32 | ||
| perRPCCreds credentials.PerRPCCredentials | ||
| customDialOptions []grpc.DialOption | ||
| resolverScheme string | ||
| clientNopCompression bool | ||
| unaryClientInt grpc.UnaryClientInterceptor | ||
| streamClientInt grpc.StreamClientInterceptor | ||
| clientInitialWindowSize int32 | ||
| clientUseInitialStreamWindowSize bool | ||
| clientInitialConnWindowSize int32 | ||
| clientStaticWindow bool | ||
| perRPCCreds credentials.PerRPCCredentials | ||
| customDialOptions []grpc.DialOption | ||
| resolverScheme string | ||
|
|
||
| // These are set once startServer is called. The common case is to have | ||
| // only one testServer. | ||
|
|
@@ -606,11 +609,21 @@ func (te *test) listenAndServe(ts testgrpc.TestServiceServer, listen func(networ | |
| sopts = append(sopts, grpc.UnknownServiceHandler(te.unknownHandler)) | ||
| } | ||
| if te.serverInitialWindowSize > 0 { | ||
| sopts = append(sopts, grpc.InitialWindowSize(te.serverInitialWindowSize)) | ||
| if te.isServerStaticWindow { | ||
| sopts = append(sopts, grpc.StaticStreamWindowSize(te.serverInitialWindowSize)) | ||
| } else { | ||
| sopts = append(sopts, grpc.InitialWindowSize(te.serverInitialWindowSize)) | ||
| } | ||
| } | ||
|
|
||
| if te.serverInitialConnWindowSize > 0 { | ||
| sopts = append(sopts, grpc.InitialConnWindowSize(te.serverInitialConnWindowSize)) | ||
| if te.isServerStaticWindow { | ||
| sopts = append(sopts, grpc.StaticConnWindowSize(te.serverInitialConnWindowSize)) | ||
| } else { | ||
| sopts = append(sopts, grpc.InitialConnWindowSize(te.serverInitialConnWindowSize)) | ||
| } | ||
| } | ||
|
|
||
| la := ":0" | ||
| if te.e.network == "unix" { | ||
| la = "/tmp/testsock" + fmt.Sprintf("%d", time.Now().UnixNano()) | ||
|
|
@@ -817,10 +830,21 @@ func (te *test) configDial(opts ...grpc.DialOption) ([]grpc.DialOption, string) | |
| opts = append(opts, grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, te.e.balancer))) | ||
| } | ||
| if te.clientInitialWindowSize > 0 { | ||
| opts = append(opts, grpc.WithInitialWindowSize(te.clientInitialWindowSize)) | ||
| if te.clientStaticWindow { | ||
| opts = append(opts, grpc.WithStaticStreamWindowSize(te.clientInitialWindowSize)) | ||
| } else if te.clientUseInitialStreamWindowSize { | ||
| opts = append(opts, grpc.WithInitialStreamWindowSize(te.clientInitialWindowSize)) | ||
| } else { | ||
| opts = append(opts, grpc.WithInitialWindowSize(te.clientInitialWindowSize)) | ||
|
Contributor
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. We should add tests with both WithInitialStreamWindowSize and WithInitialWindowSize options to ensure both code paths are verified if they diverge in future.
Member
Author
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. Done. |
||
| } | ||
| } | ||
|
|
||
| if te.clientInitialConnWindowSize > 0 { | ||
| opts = append(opts, grpc.WithInitialConnWindowSize(te.clientInitialConnWindowSize)) | ||
| if te.clientStaticWindow { | ||
| opts = append(opts, grpc.WithStaticConnWindowSize(te.clientInitialConnWindowSize)) | ||
| } else { | ||
| opts = append(opts, grpc.WithInitialConnWindowSize(te.clientInitialConnWindowSize)) | ||
| } | ||
| } | ||
| if te.perRPCCreds != nil { | ||
| opts = append(opts, grpc.WithPerRPCCredentials(te.perRPCCreds)) | ||
|
|
@@ -5418,18 +5442,23 @@ func (s) TestClientWriteFailsAfterServerClosesStream(t *testing.T) { | |
| } | ||
|
|
||
| type windowSizeConfig struct { | ||
| serverStream int32 | ||
| serverConn int32 | ||
| clientStream int32 | ||
| clientConn int32 | ||
| serverStream int32 | ||
| serverConn int32 | ||
| clientStream int32 | ||
| clientConn int32 | ||
| serverStaticWindow bool | ||
| clientStaticWindow bool | ||
|
Comment on lines
+5449
to
+5450
Contributor
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. Please rename the variables to be similar to |
||
| clientUseInitialStreamWindowSize bool | ||
| } | ||
|
|
||
| func (s) TestConfigurableWindowSizeWithLargeWindow(t *testing.T) { | ||
| wc := windowSizeConfig{ | ||
| serverStream: 8 * 1024 * 1024, | ||
| serverConn: 12 * 1024 * 1024, | ||
| clientStream: 6 * 1024 * 1024, | ||
| clientConn: 8 * 1024 * 1024, | ||
| serverStream: 8 * 1024 * 1024, | ||
| serverConn: 12 * 1024 * 1024, | ||
| clientStream: 6 * 1024 * 1024, | ||
| clientConn: 8 * 1024 * 1024, | ||
| serverStaticWindow: true, | ||
| clientStaticWindow: true, | ||
| } | ||
| for _, e := range listTestEnv() { | ||
| testConfigurableWindowSize(t, e, wc) | ||
|
|
@@ -5448,12 +5477,43 @@ func (s) TestConfigurableWindowSizeWithSmallWindow(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func (s) TestConfigurableInitialStreamWindowSizeWithLargeWindow(t *testing.T) { | ||
| wc := windowSizeConfig{ | ||
| serverStream: 8 * 1024 * 1024, | ||
| serverConn: 12 * 1024 * 1024, | ||
| clientStream: 6 * 1024 * 1024, | ||
| clientConn: 8 * 1024 * 1024, | ||
| serverStaticWindow: true, | ||
| clientStaticWindow: true, | ||
| clientUseInitialStreamWindowSize: true, | ||
| } | ||
| for _, e := range listTestEnv() { | ||
| testConfigurableWindowSize(t, e, wc) | ||
|
Contributor
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. Does this test explicitly assert the static/dynamic behavior of the client/server windows? I would expect the test to break if the window type changes unexpectedly. Is that currently covered? |
||
| } | ||
| } | ||
|
|
||
| func (s) TestConfigurableInitialStreamWindowSizeWithSmallWindow(t *testing.T) { | ||
| wc := windowSizeConfig{ | ||
| serverStream: 1, | ||
| serverConn: 1, | ||
| clientStream: 1, | ||
| clientConn: 1, | ||
| clientUseInitialStreamWindowSize: true, | ||
| } | ||
| for _, e := range listTestEnv() { | ||
| testConfigurableWindowSize(t, e, wc) | ||
| } | ||
| } | ||
|
|
||
| func testConfigurableWindowSize(t *testing.T, e env, wc windowSizeConfig) { | ||
| te := newTest(t, e) | ||
| te.serverInitialWindowSize = wc.serverStream | ||
| te.serverInitialConnWindowSize = wc.serverConn | ||
| te.clientInitialWindowSize = wc.clientStream | ||
| te.clientInitialConnWindowSize = wc.clientConn | ||
| te.isServerStaticWindow = wc.serverStaticWindow | ||
| te.clientStaticWindow = wc.clientStaticWindow | ||
| te.clientUseInitialStreamWindowSize = wc.clientUseInitialStreamWindowSize | ||
|
|
||
| te.startServer(&testServer{security: e.security}) | ||
| defer te.tearDown() | ||
|
|
||
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.
Similar to the server side boolean, please change re-name to
isClientWindowStatic