grpc: introduce new DialOption and ServerOption to configure initial window size without disabling BDP estimation. #8283

vinothkumarr227 · 2025-05-02T06:15:47Z

Part of: #7923

RELEASE NOTES:

grpc: introduce new DialOptions and ServerOptions (WithStaticStreamWindowSize, WithStaticConnWindowSize, StaticStreamWindowSize, StaticConnWindowSize) that force fixed window sizes for all HTTP/2 connections. By default, gRPC uses dynamic sizing of these windows based upon a BDP estimation algorithm. The existing options (WithInitialWindowSize, etc) also disable BDP estimation, but this behavior will be changed in a following release.

codecov · 2025-05-02T06:20:40Z

Codecov Report

Attention: Patch coverage is 33.33333% with 18 lines in your changes missing coverage. Please review.

Project coverage is 82.17%. Comparing base (32e57de) to head (6d355fb).
Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
dialoptions.go	25.00%	9 Missing ⚠️
server.go	30.76%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8283      +/-   ##
==========================================
- Coverage   82.35%   82.17%   -0.18%     
==========================================
  Files         419      419              
  Lines       42034    42073      +39     
==========================================
- Hits        34615    34572      -43     
- Misses       5968     6029      +61     
- Partials     1451     1472      +21

Files with missing lines	Coverage Δ
internal/transport/http2_client.go	`92.64% <100.00%> (+0.48%)`	⬆️
internal/transport/http2_server.go	`91.59% <100.00%> (+0.50%)`	⬆️
internal/transport/transport.go	`91.56% <ø> (ø)`
dialoptions.go	`87.40% <25.00%> (-3.40%)`	⬇️
server.go	`81.45% <30.76%> (-1.46%)`	⬇️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

arjan-bal · 2025-05-05T09:18:03Z

Is this the same as #8279? Should the original PR be closed?

vinothkumarr227 · 2025-05-05T09:21:53Z

Is this the same as #8279? Should the original PR be closed?
Yeah, you can close this PR: #8279.

vinothkumarr227 · 2025-05-07T09:08:57Z

@dfawley Can you please review this when you get a chance?

dfawley

This PR should implement just (1) from #7923.

It should not mark the old options as deprecated.

Please improve the PR title ("the size"?)
Please write release notes

Thanks!

vinothkumarr227 · 2025-05-13T07:35:09Z

This PR should implement just (1) from #7923.

It should not mark the old options as deprecated.

Please improve the PR title ("the size"?)

Please write release notes

Thanks!

@dfawley Could you please re-review this PR? I've made the changes also updated the PR title and added the release notes as suggested.

dfawley

And please update the first comment and release notes in this PR accordingly.

dfawley · 2025-05-20T22:16:55Z

default_dial_option_server_option_test.go

+// TestJoinDialOption_StaticConnAndStreamWindowSizes verifies that both static
+// stream and connection window sizes set via joined dial options are correctly
+// applied.


Is there some special interaction here with these and Join? I don't think there's much need for this test, since it just tests Join, but we already have a test for that.

dfawley · 2025-05-20T22:32:41Z

dialoptions.go

+// WithStaticStreamWindowSize returns a DialOption to set the static initial
+// stream window size.


Suggested change

// WithStaticStreamWindowSize returns a DialOption to set the static initial

// stream window size.

// WithStaticStreamWindowSize returns a DialOption which sets the initial

// stream window size to the value provided and prevents dynamic flow control

// from adjusting it.

internal/transport/transport.go

dfawley · 2025-05-27T22:48:54Z

dialoptions.go

+// WithStaticStreamWindowSize returns a DialOption which sets the initial
+// stream window size to the value provided and prevents dynamic flow control
+// from adjusting it


Suggested change

// WithStaticStreamWindowSize returns a DialOption which sets the initial

// stream window size to the value provided and prevents dynamic flow control

// from adjusting it

// WithStaticStreamWindowSize returns a DialOption which sets the initial

// stream window size to the value provided and disables dynamic flow control.

dfawley · 2025-05-27T22:49:09Z

dialoptions.go

+// WithStaticConnWindowSize returns a DialOption which sets the initial
+// connection window size to the value provided and prevents dynamic flow control
+// from adjusting it


Suggested change

// WithStaticConnWindowSize returns a DialOption which sets the initial

// connection window size to the value provided and prevents dynamic flow control

// from adjusting it

// WithStaticConnWindowSize returns a DialOption which sets the initial

// connection window size to the value provided and disables dynamic flow control.

dfawley · 2025-05-27T22:50:54Z

internal/transport/transport.go

+	// BdpEstimationEnabled controls whether dynamic window sizing is enabled.
+	BdpEstimationEnabled bool


If you are adding this boolean, then you should leave the names of the other fields as Initial. This boolean will now determine whether they are initial or static. I was picturing ending up with four variables, two for initial and two for static, but this way is probably better - just needs better naming.

Please address first part of this comment and change the variables back form static to initial

Done the changes

dfawley · 2025-05-27T22:53:43Z

server.go

+// StaticStreamWindowSize returns a ServerOption to set the initial stream
+// window size to the value provided and prevents dynamic flow control
+// from adjusting it.


Suggested change

// StaticStreamWindowSize returns a ServerOption to set the initial stream

// window size to the value provided and prevents dynamic flow control

// from adjusting it.

// StaticStreamWindowSize returns a ServerOption to set the initial stream

// window size to the value provided and disables dynamic flow control.

// The lower bound for window size is 64K and any value smaller than that

// will be ignored.

dfawley · 2025-05-27T22:54:04Z

server.go

+// StaticConnWindowSize returns a ServerOption to set the initial connection
+// window size to the value provided and prevents dynamic flow control
+// from adjusting it.


Suggested change

// StaticConnWindowSize returns a ServerOption to set the initial connection

// window size to the value provided and prevents dynamic flow control

// from adjusting it.

// StaticConnWindowSize returns a ServerOption to set the initial connection

// window size to the value provided and disables dynamic flow control.

// The lower bound for window size is 64K and any value smaller than that

// will be ignored.

dfawley · 2025-05-28T22:35:43Z

server.go

+	staticWindowSize       int32
+	staticConnWindowSize   int32


Please remove.

dfawley · 2025-05-29T16:38:52Z

internal/transport/http2_client.go

 	icwz := int32(initialWindowSize)
 	if opts.InitialConnWindowSize >= defaultWindowSize {
 		icwz = opts.InitialConnWindowSize
-		dynamicWindow = false
+		dynamicWindow = opts.UseDynamicWindowSizing
 	}


This logic is wrong now.

dynamicWindow can be removed, and opts.Use... can be used in its place. Same on the server.

Thanks, I've updated the code accordingly

dfawley · 2025-05-29T20:22:44Z

internal/transport/http2_client.go

@@ -309,11 +309,11 @@ func NewHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts
 			scheme = "https"
 		}
 	}
-	dynamicWindow := true
+	opts.UseDynamicWindowSizing = true


Don't alter the setting here. It's already set from the user. Setting this undoes it.

Did you run into problems with tests? Those tests would need to set this option accordingly. Also the below suggestion might help some, too.

Yes, I was facing issues in the tests. I've now removed the setting and updated the tests to set the option explicitly as needed.

dfawley · 2025-05-29T20:37:38Z

internal/transport/transport.go

+	MaxHeaderListSize      *uint32
+	HeaderTableSize        *uint32
+	BufferPool             mem.BufferPool
+	UseDynamicWindowSizing bool


The problem with this name is the default behavior (disabled) is not our current default (enabled).

So let's rename these to StaticWindowSize please.

Thanks for the feedback!, I've renamed it to StaticWindowSize for clarity and to better reflect the actual default behavior. The code has been updated accordingly.

dfawley · 2025-06-02T15:36:57Z

dialoptions.go

@@ -213,6 +213,7 @@ func WithReadBufferSize(s int) DialOption {
 func WithInitialWindowSize(s int32) DialOption {
 	return newFuncDialOption(func(o *dialOptions) {
 		o.copts.InitialWindowSize = s
+		o.copts.StaticWindowSize = false


These (4) need to be true or you will be changing behavior, which is not what we want yet.

Thanks for the clarification!, I've updated the code so that all four values are set to true

dfawley · 2025-06-02T15:37:57Z

internal/transport/http2_client.go

 	icwz := int32(initialWindowSize)
 	if opts.InitialConnWindowSize >= defaultWindowSize {
 		icwz = opts.InitialConnWindowSize
-		dynamicWindow = false
+		opts.StaticWindowSize = false


Don't alter the setting.

dfawley · 2025-06-02T15:38:19Z

internal/transport/http2_client.go

 	}
-	if dynamicWindow {
+	if opts.StaticWindowSize {


Suggested change

if opts.StaticWindowSize {

if !opts.StaticWindowSize {

dfawley

Thank you!

Add new Dial/ServerOptions to set the size

0c8d8c3

vinothkumarr227 added 3 commits May 2, 2025 07:27

Added test and fix vet errors

3f776f2

small tweaks

a55c070

Added test

21f3aae

arjan-bal assigned vinothkumarr227 May 5, 2025

arjan-bal added the Status: Requires Reporter Clarification label May 5, 2025

arjan-bal mentioned this pull request May 5, 2025

grpc: add new Dial/ServerOptions to set the size like the existing ones, but which do not also disable BDP estimation #8279

Closed

purnesh42H requested a review from dfawley May 9, 2025 11:17

purnesh42H assigned dfawley and unassigned vinothkumarr227 May 9, 2025

dfawley requested changes May 9, 2025

View reviewed changes

dfawley assigned vinothkumarr227 and unassigned dfawley May 9, 2025

vinothkumarr227 changed the title ~~Add new Dial/ServerOptions to set the size~~ grpc: add DialOption and ServerOption to configure initial window size without disabling BDP estimation May 12, 2025

Fixed the review changes

68f1322

vinothkumarr227 requested a review from dfawley May 13, 2025 06:25

purnesh42H changed the title ~~grpc: add DialOption and ServerOption to configure initial window size without disabling BDP estimation~~ grpc: introduce new DialOption and ServerOption to configure initial window size without disabling BDP estimation. May 13, 2025

purnesh42H added Type: Feature Area: RPC Features and removed Status: Requires Reporter Clarification labels May 13, 2025

purnesh42H added this to the 1.73 Release milestone May 13, 2025

purnesh42H assigned dfawley and unassigned vinothkumarr227 May 13, 2025

dfawley reviewed May 21, 2025

View reviewed changes

dfawley assigned vinothkumarr227 May 21, 2025

dfawley reviewed May 27, 2025

View reviewed changes

dfawley removed their assignment May 27, 2025

Fixed the review changes

a3c4701

vinothkumarr227 requested a review from dfawley May 28, 2025 12:03

Fixed the review changes

9abcf34

dfawley reviewed May 28, 2025

View reviewed changes

Remove static

838385e

vinothkumarr227 requested a review from dfawley May 29, 2025 15:16

eshitachandwani assigned dfawley and unassigned vinothkumarr227 May 29, 2025

dfawley reviewed May 29, 2025

View reviewed changes

dfawley assigned vinothkumarr227 and unassigned dfawley May 29, 2025

vinothkumarr227 added 3 commits May 29, 2025 17:02

Fixed the dynamic window size isssues

7ba8859

Fixed the test cases

382ef30

Fixed the window size

48c91da

vinothkumarr227 requested a review from dfawley May 29, 2025 17:36

dfawley reviewed May 29, 2025

View reviewed changes

vinothkumarr227 added 2 commits May 30, 2025 11:35

Fixed the review changes

cf685db

Fixed the test cases

35a2253

vinothkumarr227 requested a review from dfawley June 2, 2025 08:25

dfawley reviewed Jun 2, 2025

View reviewed changes

vinothkumarr227 added 3 commits June 2, 2025 17:53

Fixed the review changes

6ec9e7d

small tweaks

efe83f4

Fixed the test cases

6d355fb

vinothkumarr227 requested a review from dfawley June 2, 2025 18:24

dfawley approved these changes Jun 2, 2025

View reviewed changes

dfawley merged commit 9b7bd34 into grpc:master Jun 2, 2025
15 checks passed

This was referenced Jun 25, 2025

Feature implementation from commits 28128e0..d2e8366 yashuatla/grpc-go#1

Open

Feature implementation from commits 6995ef2..f6bf86c yashuatla/grpc-go#2

Open

		// WithStaticStreamWindowSize returns a DialOption to set the static initial
		// stream window size.

		// BdpEstimationEnabled controls whether dynamic window sizing is enabled.
		BdpEstimationEnabled bool

grpc: introduce new DialOption and ServerOption to configure initial window size without disabling BDP estimation. #8283

grpc: introduce new DialOption and ServerOption to configure initial window size without disabling BDP estimation. #8283

Uh oh!

Conversation

vinothkumarr227 commented May 2, 2025 • edited by dfawley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arjan-bal commented May 5, 2025

Uh oh!

vinothkumarr227 commented May 5, 2025

Uh oh!

vinothkumarr227 commented May 7, 2025

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

vinothkumarr227 commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vinothkumarr227 commented May 2, 2025 •

edited by dfawley

Loading

codecov bot commented May 2, 2025 •

edited

Loading

vinothkumarr227 commented May 13, 2025 •

edited

Loading