[GitHub] [trafficserver] bneradt commented on a diff in pull request #9117: Fix HTTP/2 session receive window handling for small sizes

[GitBox <gi...@apache.org>]

bneradt commented on code in PR #9117:
URL: https://github.com/apache/trafficserver/pull/9117#discussion_r985951880


##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1039,8 +1039,18 @@ Http2ConnectionState::Http2ConnectionState() : stream_list()
 void
 Http2ConnectionState::init(Http2CommonSession *ssn)
 {
-  session            = ssn;
-  this->_server_rwnd = Http2::initial_window_size;
+  session = ssn;
+
+  if (Http2::initial_window_size < HTTP2_INITIAL_WINDOW_SIZE) {
+    // There is no HTTP/2 specified way to shrink the connection window size
+    // other than to receive data and not send WINDOW_UPDATE frames for a
+    // while.
+    this->_server_rwnd              = HTTP2_INITIAL_WINDOW_SIZE;

Review Comment:
   `HTTP2_INITIAL_WINDOW_SIZE` is actually the 65,535 value, so this isn't the lower but rather the higher value. So here's the function:
   
   ```
    if (Http2::initial_window_size < HTTP2_INITIAL_WINDOW_SIZE) {
        // There is no HTTP/2 specified way to shrink the connection window size
        // other than to receive data and not send WINDOW_UPDATE frames for a
        // while.
        this->_server_rwnd              = HTTP2_INITIAL_WINDOW_SIZE;
        this->_server_rwnd_is_shrinking = true;
      } else {
        this->_server_rwnd              = Http2::initial_window_size;
        this->_server_rwnd_is_shrinking = false;
      }
   ```
   
   * If, per the first branch, the user configured a _smaller_ value than the spec's default session window size (`HTTP2_INITIAL_WINDOW_SIZE`, 65,535), then we initialize the window to that higher value and indicate that we are actively shrinking the window to the desired state. `restart_receiving` already handles this appropriately without any change in this patch...it sends `WINDOW_UPDATE` frames when the receive session window drops below the configured session window (not this default window).
   * If the user configures a session window _larger_ than the default window size of 65,535 (the else case above), then we can simply set the `_server_rwnd` to the larger value and, later in the function, we send a `WINDOW_UPDATE` frame to tell the peer to increase the session window size as desired.



##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1039,8 +1039,18 @@ Http2ConnectionState::Http2ConnectionState() : stream_list()
 void
 Http2ConnectionState::init(Http2CommonSession *ssn)
 {
-  session            = ssn;
-  this->_server_rwnd = Http2::initial_window_size;
+  session = ssn;
+
+  if (Http2::initial_window_size < HTTP2_INITIAL_WINDOW_SIZE) {
+    // There is no HTTP/2 specified way to shrink the connection window size
+    // other than to receive data and not send WINDOW_UPDATE frames for a
+    // while.
+    this->_server_rwnd              = HTTP2_INITIAL_WINDOW_SIZE;

Review Comment:
   Thank you for the review. Let me know whether I misunderstood your question.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org