[GitHub] [trafficserver] maskit commented on a diff in pull request #8963: Http2 to origin

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

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


##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1275,6 +1331,99 @@ Http2ConnectionState::state_closed(int event, void *edata)
   return 0;
 }
 
+bool
+Http2ConnectionState::is_peer_concurrent_stream_ub() const
+{
+  return client_streams_in_count >= (peer_settings.get(HTTP2_SETTINGS_MAX_CONCURRENT_STREAMS)) * 0.9;

Review Comment:
   I don't understand why these magic thresholds are needed. Although an implementation must not exceed the limit on purpose, it doesn't need to consider a possibility that a limit change is on wire.
   
   > The values in the SETTINGS frame MUST be processed in the order they
      appear, with no other frame processing between values.  Unsupported
      settings MUST be ignored.  Once all values have been processed, the
      recipient MUST immediately emit a SETTINGS frame with the ACK flag
      set.  Upon receiving a SETTINGS frame with the ACK flag set, the
      sender of the altered settings can rely on the values from the oldest
      unacknowledged SETTINGS frame having been applied.
   
   >   If the sender of a SETTINGS frame does not receive an acknowledgment
      within a reasonable amount of time, it MAY issue a connection error
      ([Section 5.4.1](https://datatracker.ietf.org/doc/html/rfc9113#section-5.4.1)) of type SETTINGS_TIMEOUT.  In setting a timeout, some
      allowance needs to be made for processing delays at the peer; a
      timeout that is solely based on the round-trip time between endpoints
      might result in spurious errors.
   
   The RFC also says:
   > An endpoint that wishes to reduce the value of
      SETTINGS_MAX_CONCURRENT_STREAMS to a value that is below the current
      number of open streams can either close streams that exceed the new
      value or allow streams to complete.
   
   It's not a something that need to be taken care by stream initiator



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