Fix connection-level flow control (local)

Fix the bug that connection-level local window is not updated
for the data is the last part of the stream. For the stream
level window may ignore this, connection-level window must
be updated. Also this change fixes the bug that connection-level
window is not updated for the ignored DATA frames.

lib/nghttp2_session.c

@@ -2556,9 +2556,10 @@ static int32_t adjust_recv_window_size(int32_t recv_window_size, int32_t delta)
 }
 
 /*
- * Accumulates received bytes |delta_size| and decides whether to send
- * WINDOW_UPDATE. If NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE is set,
- * WINDOW_UPDATE will not be sent.
+ * Accumulates received bytes |delta_size| for stream-level flow
+ * control and decides whether to send WINDOW_UPDATE to that
+ * stream. If NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE is set, WINDOW_UPDATE
+ * will not be sent.
  *
  * This function returns 0 if it succeeds, or one of the following
  * negative error codes:
@@ -2566,51 +2567,67 @@ static int32_t adjust_recv_window_size(int32_t recv_window_size, int32_t delta)
  * NGHTTP2_ERR_NOMEM
  *     Out of memory.
  */
-static int nghttp2_session_update_recv_window_size(nghttp2_session *session,
-                                                   nghttp2_stream *stream,
-                                                   int32_t delta_size)
-{
-  if(stream && stream->local_flow_control) {
-    stream->recv_window_size = adjust_recv_window_size
-      (stream->recv_window_size, delta_size);
-    if(!(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE)) {
-      /* This is just a heuristics. */
-      /* We have to use local_settings here because it is the constraint
-         the remote endpoint should honor. */
-      if((size_t)stream->recv_window_size*2 >=
-         session->local_settings[NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE]) {
-        int r;
-        r = nghttp2_session_add_window_update(session,
-                                              NGHTTP2_FLAG_NONE,
-                                              stream->stream_id,
-                                              stream->recv_window_size);
-        if(r == 0) {
-          stream->recv_window_size = 0;
-        } else {
-          return r;
-        }
+static int nghttp2_session_update_recv_stream_window_size
+(nghttp2_session *session,
+ nghttp2_stream *stream,
+ int32_t delta_size)
+{
+  stream->recv_window_size = adjust_recv_window_size
+    (stream->recv_window_size, delta_size);
+  if(!(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE)) {
+    /* This is just a heuristics. */
+    /* We have to use local_settings here because it is the constraint
+       the remote endpoint should honor. */
+    if((size_t)stream->recv_window_size*2 >=
+       session->local_settings[NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE]) {
+      int r;
+      r = nghttp2_session_add_window_update(session,
+                                            NGHTTP2_FLAG_NONE,
+                                            stream->stream_id,
+                                            stream->recv_window_size);
+      if(r == 0) {
+        stream->recv_window_size = 0;
+      } else {
+        return r;
       }
     }
   }
-  if(session->local_flow_control) {
-    session->recv_window_size = adjust_recv_window_size
-      (session->recv_window_size, delta_size);
-    if(!(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE)) {
-      /* Same heuristics above */
-      if((size_t)session->recv_window_size*2 >=
-         NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) {
-        int r;
-        /* Use stream ID 0 to update connection-level flow control
-           window */
-        r = nghttp2_session_add_window_update(session,
-                                              NGHTTP2_FLAG_NONE,
-                                              0,
-                                              session->recv_window_size);
-        if(r == 0) {
-          session->recv_window_size = 0;
-        } else {
-          return r;
-        }
+  return 0;
+}
+
+/*
+ * Accumulates received bytes |delta_size| for connection-level flow
+ * control and decides whether to send WINDOW_UPDATE to the
+ * connection.  If NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE is set,
+ * WINDOW_UPDATE will not be sent.
+ *
+ * This function returns 0 if it succeeds, or one of the following
+ * negative error codes:
+ *
+ * NGHTTP2_ERR_NOMEM
+ *     Out of memory.
+ */
+static int nghttp2_session_update_recv_connection_window_size
+(nghttp2_session *session,
+ int32_t delta_size)
+{
+  session->recv_window_size = adjust_recv_window_size
+    (session->recv_window_size, delta_size);
+  if(!(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE)) {
+    /* Same heuristics above */
+    if((size_t)session->recv_window_size*2 >=
+       NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) {
+      int r;
+      /* Use stream ID 0 to update connection-level flow control
+         window */
+      r = nghttp2_session_add_window_update(session,
+                                            NGHTTP2_FLAG_NONE,
+                                            0,
+                                            session->recv_window_size);
+      if(r == 0) {
+        session->recv_window_size = 0;
+      } else {
+        return r;
       }
     }
   }
@@ -2727,28 +2744,40 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session,
                                                            session->user_data);
           }
         }
+        /* TODO We need on_ignored_data_chunk_recv_callback, for
+           ingored DATA frame, so that the application can calculate
+           connection-level window size. */
       }
       session->iframe.off += readlen;
       inmark += readlen;
 
-      if(session->iframe.state != NGHTTP2_RECV_PAYLOAD_IGN &&
-         nghttp2_frame_is_data_frame(session->iframe.headbuf) &&
-         readlen > 0 &&
-         (session->iframe.payloadlen != session->iframe.off ||
-          (data_flags & NGHTTP2_FLAG_END_STREAM) == 0)) {
-        nghttp2_stream *stream;
-        stream = nghttp2_session_get_stream(session, data_stream_id);
-        if(session->local_flow_control ||
-           (stream && stream->local_flow_control)) {
-          r = nghttp2_session_update_recv_window_size(session,
-                                                      stream,
-                                                      readlen);
+      if(nghttp2_frame_is_data_frame(session->iframe.headbuf) && readlen > 0) {
+        if(session->local_flow_control) {
+          /* Update connection-level flow control window for ignored
+             DATA frame too */
+          r = nghttp2_session_update_recv_connection_window_size
+            (session, readlen);
           if(r < 0) {
             /* FATAL */
             assert(r < NGHTTP2_ERR_FATAL);
             return r;
           }
         }
+        if(session->iframe.state != NGHTTP2_RECV_PAYLOAD_IGN &&
+           (session->iframe.payloadlen != session->iframe.off ||
+            (data_flags & NGHTTP2_FLAG_END_STREAM) == 0)) {
+          nghttp2_stream *stream;
+          stream = nghttp2_session_get_stream(session, data_stream_id);
+          if(stream && stream->local_flow_control) {
+            r = nghttp2_session_update_recv_stream_window_size
+              (session, stream, readlen);
+            if(r < 0) {
+              /* FATAL */
+              assert(r < NGHTTP2_ERR_FATAL);
+              return r;
+            }
+          }
+        }
       }
       if(session->iframe.payloadlen == session->iframe.off) {
         if(!nghttp2_frame_is_data_frame(session->iframe.headbuf)) {

tests/main.c

@@ -170,6 +170,8 @@ int main(int argc, char* argv[])
                    test_nghttp2_session_flow_control_disable_remote) ||
       !CU_add_test(pSuite, "session_flow_control_disable_local",
                    test_nghttp2_session_flow_control_disable_local) ||
+      !CU_add_test(pSuite, "session_flow_control_data_recv",
+                   test_nghttp2_session_flow_control_data_recv) ||
       !CU_add_test(pSuite, "session_data_read_temporal_failure",
                    test_nghttp2_session_data_read_temporal_failure) ||
       !CU_add_test(pSuite, "session_on_request_recv_callback",

tests/nghttp2_session_test.c

@@ -2614,6 +2614,65 @@ void test_nghttp2_session_flow_control_disable_local(void)
   nghttp2_session_del(session);
 }
 
+void test_nghttp2_session_flow_control_data_recv(void)
+{
+  nghttp2_session *session;
+  nghttp2_session_callbacks callbacks;
+  uint8_t data[64*1024+16];
+  nghttp2_frame_hd hd;
+  nghttp2_outbound_item *item;
+
+  memset(&callbacks, 0, sizeof(nghttp2_session_callbacks));
+  callbacks.send_callback = null_send_callback;
+
+  /* Initial window size to 64KiB - 1*/
+  nghttp2_session_client_new(&session, &callbacks, NULL);
+
+  nghttp2_session_open_stream(session, 1, NGHTTP2_FLAG_NONE,
+                              NGHTTP2_PRI_DEFAULT, NGHTTP2_STREAM_OPENED,
+                              NULL);
+
+  /* Create DATA frame */
+  memset(data, 0, sizeof(data));
+  hd.length = NGHTTP2_MAX_FRAME_LENGTH;
+  hd.type = NGHTTP2_DATA;
+  hd.flags = NGHTTP2_FLAG_END_STREAM;
+  hd.stream_id = 1;
+  nghttp2_frame_pack_frame_hd(data, &hd);
+  CU_ASSERT(NGHTTP2_MAX_FRAME_LENGTH+NGHTTP2_FRAME_HEAD_LENGTH ==
+            nghttp2_session_mem_recv(session, data,
+                                     NGHTTP2_MAX_FRAME_LENGTH +
+                                     NGHTTP2_FRAME_HEAD_LENGTH));
+
+  item = nghttp2_session_get_next_ob_item(session);
+  /* Since this is the last frame, stream-level WINDOW_UPDATE is not
+     issued, but connection-level does. */
+  CU_ASSERT(NGHTTP2_WINDOW_UPDATE == OB_CTRL_TYPE(item));
+  CU_ASSERT(0 == OB_CTRL(item)->hd.stream_id);
+  CU_ASSERT(NGHTTP2_MAX_FRAME_LENGTH ==
+            OB_CTRL(item)->window_update.window_size_increment);
+
+  CU_ASSERT(0 == nghttp2_session_send(session));
+
+  /* Receive DATA for closed stream. They are still subject to under
+     connection-level flow control, since this situation arises when
+     RST_STREAM is issued by the remote, but the local side keeps
+     sending DATA frames. Without calculating connection-level window,
+     the subsequent flow control gets confused. */
+  CU_ASSERT(NGHTTP2_MAX_FRAME_LENGTH+NGHTTP2_FRAME_HEAD_LENGTH ==
+            nghttp2_session_mem_recv(session, data,
+                                     NGHTTP2_MAX_FRAME_LENGTH +
+                                     NGHTTP2_FRAME_HEAD_LENGTH));
+
+  item = nghttp2_session_get_next_ob_item(session);
+  CU_ASSERT(NGHTTP2_WINDOW_UPDATE == OB_CTRL_TYPE(item));
+  CU_ASSERT(0 == OB_CTRL(item)->hd.stream_id);
+  CU_ASSERT(NGHTTP2_MAX_FRAME_LENGTH ==
+            OB_CTRL(item)->window_update.window_size_increment);
+
+  nghttp2_session_del(session);
+}
+
 void test_nghttp2_session_data_read_temporal_failure(void)
 {
   nghttp2_session *session;

tests/nghttp2_session_test.h

@@ -76,6 +76,7 @@ void test_nghttp2_session_defer_data(void);
 void test_nghttp2_session_flow_control(void);
 void test_nghttp2_session_flow_control_disable_remote(void);
 void test_nghttp2_session_flow_control_disable_local(void);
+void test_nghttp2_session_flow_control_data_recv(void);
 void test_nghttp2_session_data_read_temporal_failure(void);
 void test_nghttp2_session_on_request_recv_callback(void);
 void test_nghttp2_session_on_stream_close(void);