Mark stream as cancelled if we exceed the metadata size limit.

Also take this opportunity to convert the seen_error field to a bool.

4 files changed, 40 insertions, 19 deletions

diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c
index c24950a189..b73ec2a7e9 100644
--- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c
+++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c
@@ -858,7 +858,7 @@ static void perform_stream_op_locked(
         add_closure_barrier(on_complete);
     stream_global->send_initial_metadata = op->send_initial_metadata;
     if (contains_non_ok_status(transport_global, op->send_initial_metadata)) {
-      stream_global->seen_error = 1;
+      stream_global->seen_error = true;
       grpc_chttp2_list_add_check_read_ops(transport_global, stream_global);
     }
     if (!stream_global->write_closed) {
@@ -899,7 +899,7 @@ static void perform_stream_op_locked(
         add_closure_barrier(on_complete);
     stream_global->send_trailing_metadata = op->send_trailing_metadata;
     if (contains_non_ok_status(transport_global, op->send_trailing_metadata)) {
-      stream_global->seen_error = 1;
+      stream_global->seen_error = true;
       grpc_chttp2_list_add_check_read_ops(transport_global, stream_global);
     }
     if (stream_global->write_closed) {
@@ -1076,6 +1076,16 @@ static void check_read_ops(grpc_exec_ctx *exec_ctx,
       grpc_chttp2_list_pop_check_read_ops(transport_global, &stream_global)) {
     if (stream_global->recv_initial_metadata_ready != NULL &&
         stream_global->published_initial_metadata) {
+      if (stream_global->seen_error) {
+        while ((bs = grpc_chttp2_incoming_frame_queue_pop(
+                    &stream_global->incoming_frames)) != NULL) {
+          grpc_byte_stream_destroy(exec_ctx, bs);
+        }
+        if (stream_global->exceeded_metadata_size) {
+          cancel_from_api(exec_ctx, transport_global, stream_global,
+                          GRPC_STATUS_RESOURCE_EXHAUSTED);
+        }
+      }
       grpc_chttp2_incoming_metadata_buffer_publish(
           &stream_global->received_initial_metadata,
           stream_global->recv_initial_metadata);
@@ -1105,10 +1115,15 @@ static void check_read_ops(grpc_exec_ctx *exec_ctx,
     }
     if (stream_global->recv_trailing_metadata_finished != NULL &&
         stream_global->read_closed && stream_global->write_closed) {
-      while (stream_global->seen_error &&
-             (bs = grpc_chttp2_incoming_frame_queue_pop(
-                  &stream_global->incoming_frames)) != NULL) {
-        grpc_byte_stream_destroy(exec_ctx, bs);
+      if (stream_global->seen_error) {
+        while ((bs = grpc_chttp2_incoming_frame_queue_pop(
+                    &stream_global->incoming_frames)) != NULL) {
+          grpc_byte_stream_destroy(exec_ctx, bs);
+        }
+        if (stream_global->exceeded_metadata_size) {
+          cancel_from_api(exec_ctx, transport_global, stream_global,
+                          GRPC_STATUS_RESOURCE_EXHAUSTED);
+        }
       }
       if (stream_global->incoming_frames.head == NULL) {
         grpc_chttp2_incoming_metadata_buffer_publish(
@@ -1175,7 +1190,7 @@ static void cancel_from_api(grpc_exec_ctx *exec_ctx,
                             NULL);
   }
   if (status != GRPC_STATUS_OK && !stream_global->seen_error) {
-    stream_global->seen_error = 1;
+    stream_global->seen_error = true;
     grpc_chttp2_list_add_check_read_ops(transport_global, stream_global);
   }
   grpc_chttp2_mark_stream_closed(exec_ctx, transport_global, stream_global, 1,
@@ -1187,7 +1202,7 @@ void grpc_chttp2_fake_status(grpc_exec_ctx *exec_ctx,
                              grpc_chttp2_stream_global *stream_global,
                              grpc_status_code status, gpr_slice *slice) {
   if (status != GRPC_STATUS_OK) {
-    stream_global->seen_error = 1;
+    stream_global->seen_error = true;
     grpc_chttp2_list_add_check_read_ops(transport_global, stream_global);
   }
   /* stream_global->recv_trailing_metadata_finished gives us a
diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h
index d547a6e9c1..be38ffda1f 100644
--- a/src/core/ext/transport/chttp2/transport/internal.h
+++ b/src/core/ext/transport/chttp2/transport/internal.h
@@ -410,7 +410,8 @@ typedef struct {
   uint8_t in_stream_map;
   /** has this stream seen an error? if 1, then pending incoming frames
       can be thrown away */
-  uint8_t seen_error;
+  bool seen_error;
+  bool exceeded_metadata_size;
 
   uint8_t published_initial_metadata;
   uint8_t published_trailing_metadata;
@@ -457,7 +458,8 @@ struct grpc_chttp2_stream_parsing {
   /** which metadata did we get (on this parse) */
   uint8_t got_metadata_on_parse[2];
   /** should we raise the seen_error flag in transport_global */
-  uint8_t seen_error;
+  bool seen_error;
+  bool exceeded_metadata_size;
   /** window available for peer to send to us */
   int64_t incoming_window;
   /** parsing state for data frames */
diff --git a/src/core/ext/transport/chttp2/transport/parsing.c b/src/core/ext/transport/chttp2/transport/parsing.c
index 0cf4d87f3c..11cbb80ca8 100644
--- a/src/core/ext/transport/chttp2/transport/parsing.c
+++ b/src/core/ext/transport/chttp2/transport/parsing.c
@@ -167,7 +167,9 @@ void grpc_chttp2_publish_reads(
   while (grpc_chttp2_list_pop_parsing_seen_stream(
       transport_global, transport_parsing, &stream_global, &stream_parsing)) {
     if (stream_parsing->seen_error) {
-      stream_global->seen_error = 1;
+      stream_global->seen_error = true;
+      stream_global->exceeded_metadata_size =
+          stream_parsing->exceeded_metadata_size;
       grpc_chttp2_list_add_check_read_ops(transport_global, stream_global);
     }
 
@@ -603,7 +605,7 @@ static void on_initial_header(void *tp, grpc_mdelem *md) {
 
   if (md->key == GRPC_MDSTR_GRPC_STATUS && md != GRPC_MDELEM_GRPC_STATUS_0) {
     /* TODO(ctiller): check for a status like " 0" */
-    stream_parsing->seen_error = 1;
+    stream_parsing->seen_error = true;
   }
 
   if (md->key == GRPC_MDSTR_GRPC_TIMEOUT) {
@@ -627,7 +629,8 @@ static void on_initial_header(void *tp, grpc_mdelem *md) {
     const size_t new_size = stream_parsing->metadata_buffer[0].size +
                             GRPC_MDELEM_LENGTH(md);
     if (new_size > transport_parsing->max_metadata_size) {
-      stream_parsing->seen_error = 1;
+      stream_parsing->seen_error = true;
+      stream_parsing->exceeded_metadata_size = true;
       GRPC_MDELEM_UNREF(md);
     } else {
       grpc_chttp2_incoming_metadata_buffer_add(
@@ -656,13 +659,14 @@ static void on_trailing_header(void *tp, grpc_mdelem *md) {
 
   if (md->key == GRPC_MDSTR_GRPC_STATUS && md != GRPC_MDELEM_GRPC_STATUS_0) {
     /* TODO(ctiller): check for a status like " 0" */
-    stream_parsing->seen_error = 1;
+    stream_parsing->seen_error = true;
   }
 
   const size_t new_size = stream_parsing->metadata_buffer[1].size +
                           GRPC_MDELEM_LENGTH(md);
   if (new_size > transport_parsing->max_metadata_size) {
-    stream_parsing->seen_error = 1;
+    stream_parsing->seen_error = true;
+    stream_parsing->exceeded_metadata_size = true;
     GRPC_MDELEM_UNREF(md);
   } else {
     grpc_chttp2_incoming_metadata_buffer_add(
diff --git a/test/core/end2end/tests/large_metadata.c b/test/core/end2end/tests/large_metadata.c
index f09b55a2c0..2aa6381e9e 100644
--- a/test/core/end2end/tests/large_metadata.c
+++ b/test/core/end2end/tests/large_metadata.c
@@ -199,7 +199,7 @@ static void test_request_with_large_metadata(grpc_end2end_test_config config,
   error = grpc_call_start_batch(s, ops, (size_t)(op - ops), tag(102), NULL);
   GPR_ASSERT(GRPC_CALL_OK == error);
 
-  cq_expect_completion(cqv, tag(102), 1);
+  cq_expect_completion(cqv, tag(102), allow_large_metadata);
   cq_verify(cqv);
 
   op = ops;
@@ -222,13 +222,13 @@ static void test_request_with_large_metadata(grpc_end2end_test_config config,
   cq_expect_completion(cqv, tag(1), 1);
   cq_verify(cqv);
 
-// FIXME: why is this assert passing with allow_large_metadata=false?
-  GPR_ASSERT(status == GRPC_STATUS_OK);
-  GPR_ASSERT(0 == strcmp(details, "xyz"));
+  GPR_ASSERT(status == (allow_large_metadata ? GRPC_STATUS_OK
+                        : GRPC_STATUS_RESOURCE_EXHAUSTED));
   GPR_ASSERT(0 == strcmp(call_details.method, "/foo"));
   GPR_ASSERT(0 == strcmp(call_details.host, "foo.test.google.fr"));
   GPR_ASSERT(was_cancelled == 0);
   if (allow_large_metadata) {
+    GPR_ASSERT(0 == strcmp(details, "xyz"));
     GPR_ASSERT(byte_buffer_eq_string(request_payload_recv, "hello world"));
     GPR_ASSERT(contains_metadata(&request_metadata_recv, "key", meta.value));
   } else {