Make window size configurable. #1210
Conversation
clientconn.go
Outdated
| @@ -106,6 +106,20 @@ const defaultClientMaxMsgSize = math.MaxInt32 | |||
| // DialOption configures how we set up the connection. | |||
| type DialOption func(*dialOptions) | |||
|
|
|||
| // WithInitialWindowSize returns a DialOption which sets the value for initial window size on a stream. | |||
| func WithInitialWindowSize(s int32) DialOption { | |||
There was a problem hiding this comment.
should the custom intitialConnWindowSize and InitialWindowSize should be capped to something smaller, and lower-bounded to current values here?
| @@ -142,16 +145,24 @@ func newHTTP2Server(conn net.Conn, config *ServerConfig) (_ ServerTransport, err | |||
| Val: maxStreams, | |||
| }) | |||
| } | |||
| if initialWindowSize != defaultWindowSize { | |||
| iwz := int32(initialWindowSize) | |||
| if config.InitialWindowSize >= defaultWindowSize { | |||
There was a problem hiding this comment.
It looks like iwz should be reset to the custom value only if config.InitialWindowSize >= initialWindowSize, rather than the defaultWindowSize, since defaultWindowSize could potentially be less than initialWindowSize?
There was a problem hiding this comment.
The send quota on both client and server for both stream and connection takes the default value of defaultWindowSize. Therefore, that can be our lower cap. Is there an issue if it's smaller than initialWindowSize?
| iwz = config.InitialWindowSize | ||
| } | ||
| icwz := int32(initialConnWindowSize) | ||
| if config.InitialConnWindowSize >= defaultWindowSize { |
There was a problem hiding this comment.
since currently connection window is initialConnWindowSize, should icwz should be reset only if custom value is greater than initialConnWindowSize, to keep the current value as the lower limit?
There was a problem hiding this comment.
Leaving as is following offline discussion.
transport/http2_server.go
Outdated
| idle: time.Now(), | ||
| kep: kep, | ||
| initialWindowSize: iwz, | ||
| initialConnWindowSize: icwz, |
There was a problem hiding this comment.
It looks like after their inFlows are created and the connection window updates sent, the initialConnWindowSize fields are no longer used by the transport structs, can they be gotten rid of as fields in the structs?
| numOfIter := 11 | ||
| // Set message size to exhaust largest of window sizes. | ||
| messageSize := max(max(wc.serverStream, wc.serverConn), max(wc.clientStream, wc.clientConn)) / int32(numOfIter-1) | ||
| messageSize = max(messageSize, 64*1024) |
transport/transport_test.go
Outdated
| defer server.stop() | ||
| defer client.Close() | ||
| // Sleep for a second to make sure connection is established. | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
Optional nit, I think this can be done in a follow-up:
maybe this time.Sleep can be replaced with internal access to settings frame sends and acks - then this could wait on a settings frame acks. (It looks like it risks flakiness as is).
transport/transport_test.go
Outdated
| t.Fatalf("Client stream flow control window size is %v, want %v", cstream.fc.limit, connectOptions.InitialWindowSize) | ||
| } | ||
| // Sleep for a bit to make sure server received headers for the stream. | ||
| time.Sleep(time.Millisecond * 500) |
There was a problem hiding this comment.
related to comment above, maybe this sleep can be replaced by starting an RPC and then waiting for something on the server to make sure it's been started.
transport/transport_test.go
Outdated
| defer st.mu.Unlock() | ||
| return len(st.activeStreams) == 0 | ||
| }) | ||
|
|
There was a problem hiding this comment.
I still think there should be a time.Sleep(time.Second) here since there's really no good way of knowing that settings received were applied.
Can't check if streamSendQuota on transport was updated since in case of the second test that remains at the default size.
This seems flakier than before.
There was a problem hiding this comment.
Yeah I can't see any way myself to avoid the race/sleep without something "testonly".
My idea for it could be something like:
Maybe the handler that apply's settings could notify a test-only "settings applied" channel, or the same thing could be done with the "settings ack" handler in the client.
There was a problem hiding this comment.
You're right, if we introduce call back hooks in the code we can get this done. However, I don't feel very comfortable with introducing test-facilitating code in our codebase, especially for a small test like this. What do you think?
|
lgtm on latest update! |
menghanl
added
1.4
Type: Feature
No description provided.