[GitHub] [trafficserver] maskit commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable

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

maskit commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r996539311


##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4152,7 +4152,40 @@ HTTP/2 Configuration
    :reloadable:
    :units: bytes
 
-   The initial window size for inbound connections.
+   The initial HTTP/2 stream window size for inbound connections that |TS| as a
+   receiver advertises to the peer. See IETF RFC 9113 section 5.2 for details
+   concerning HTTP/2 flow control. See
+   :ts:cv:`proxy.config.http2.flow_control.in.policy` for how HTTP/2 stream and
+   session windows are maintained over the lifetime of HTTP/2 connections.

Review Comment:
   Let's not mix terms in the spec and terms in ATS. It should be Connection-Stream, or Session-Transaction.



##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1572,12 +1656,23 @@ Http2ConnectionState::release_stream()
 }
 
 void
-Http2ConnectionState::update_initial_rwnd(Http2WindowSize new_size)
+Http2ConnectionState::update_initial_client_rwnd(Http2WindowSize new_size)

Review Comment:
   How about `update_initial_peer_rwnd` ? We forgot to rename `_client_rwnd` and `_server_rwnd` on #9084. 



##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1427,31 +1477,65 @@ Http2ConnectionState::restart_streams()
 void
 Http2ConnectionState::restart_receiving(Http2Stream *stream)
 {
-  uint32_t initial_rwnd = this->local_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
-  uint32_t min_rwnd     = std::min(initial_rwnd, this->local_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE));
-
   // Connection level WINDOW UPDATE
-  if (this->server_rwnd() < min_rwnd) {
-    Http2WindowSize diff_size = initial_rwnd - this->server_rwnd();
-    this->increment_server_rwnd(diff_size);
-    this->_server_rwnd_is_shrinking = false;
-    this->send_window_update_frame(0, diff_size);
+  uint32_t const configured_session_window = this->_get_configured_receive_session_window_size_in();
+  uint32_t const min_session_window = std::min(configured_session_window, this->local_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE));
+  if (this->server_rwnd() < min_session_window) {
+    Http2WindowSize diff_size = configured_session_window - this->server_rwnd();
+    if (diff_size > 0) {
+      this->increment_server_rwnd(diff_size);
+      this->_server_rwnd_is_shrinking = false;
+      this->send_window_update_frame(HTTP2_CONNECTION_CONTROL_STREAM, diff_size);
+    }
   }
 
   // Stream level WINDOW UPDATE
-  if (stream == nullptr || stream->server_rwnd() >= min_rwnd) {
+  if (stream == nullptr || stream->server_rwnd() >= min_session_window) {
+    // There's no need to increase the stream window size if it is already big
+    // enough to hold what the stream/max frame size can receive.
     return;
   }
 
+  uint32_t initial_stream_window = this->local_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
+  if (this->_has_dynamic_stream_window()) {
+    // If our stream window size is dynamic, then we have to allow for this situation:
+    //
+    // 1. ATS initializes the stream window size to the configured value via a
+    // SETTINGS frame, say 10 bytes.
+    //
+    // 2. The peer sends a data frame of that size, again 10 bytes.
+    //
+    // 3. At the same time that the peer sends the data frame, we shrink the
+    // stream window size via a SETTINGS frame to a smaller value, say 5 bytes.
+    //
+    // 4. The peer receives the SETTINGS frame and calculates its send window
+    // to be -5 bytes.
+    //
+    // 5. ATS cannot simply send a WINDOW_UPDATE frame of 5 bytes to bring the

Review Comment:
   ATS gave 10 at the beginning and the peer already consumed 10. If ATS has already processed the 10 bytes received, we should be able to give 5. If ATS has not processed the 10 bytes, there's nothing to do here yet. SETTINGS frame doesn't affect any existing streams on the peer side.



-- 
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